Skip to content

Align model_matrix input errors and docs#352

Closed
neuralsorcerer wants to merge 10 commits intofacebookresearch:mainfrom
neuralsorcerer:doc
Closed

Align model_matrix input errors and docs#352
neuralsorcerer wants to merge 10 commits intofacebookresearch:mainfrom
neuralsorcerer:doc

Conversation

@neuralsorcerer
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings March 1, 2026 16:38
@meta-cla meta-cla bot added the cla signed label Mar 1, 2026
@neuralsorcerer neuralsorcerer added this to the balance 0.17.0 milestone Mar 1, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates documentation to better explain how model-matrix inputs are prepared, and records the doc clarification in the changelog.

Changes:

  • Expanded _prepare_input_model_matrix docstring argument descriptions (sample, target, variables, add_na, fix_columns_names).
  • Added a changelog entry under Documentation noting the clarification.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
balance/utils/model_matrix.py Clarifies _prepare_input_model_matrix argument behavior in the docstring.
CHANGELOG.md Notes the docstring clarification under the Documentation section.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

tests/test_util_model_matrix.py:299

  • This test uses pd.DataFrame() (zero rows and zero columns). Since the intent is specifically “zero rows”, consider constructing an empty frame with representative columns (e.g., pd.DataFrame(columns=[...])) so the assertion is unambiguous and continues to validate the intended contract if column-handling changes.
        # Test zero rows error:
        self.assertRaisesRegex(
            ValueError,
            "sample must have more than zero rows",
            model_matrix,
            pd.DataFrame(),
        )

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@neuralsorcerer neuralsorcerer changed the title Clarify model-matrix input docstrings Align model_matrix input errors and docs Mar 1, 2026
@neuralsorcerer neuralsorcerer requested a review from talgalili March 1, 2026 17:57
Copy link
Contributor

@talgalili talgalili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Might take a day to land.

@meta-codesync
Copy link

meta-codesync bot commented Mar 2, 2026

@talgalili has imported this pull request. If you are a Meta employee, you can view this in D94872326.

@meta-codesync
Copy link

meta-codesync bot commented Mar 2, 2026

@talgalili merged this pull request in 93c62b6.

@neuralsorcerer neuralsorcerer deleted the doc branch March 2, 2026 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants