Skip to content

Add rake target margins input#528

Closed
neuralsorcerer wants to merge 5 commits into
facebookresearch:mainfrom
neuralsorcerer:rake
Closed

Add rake target margins input#528
neuralsorcerer wants to merge 5 commits into
facebookresearch:mainfrom
neuralsorcerer:rake

Conversation

@neuralsorcerer

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings July 14, 2026 01:37
@meta-cla meta-cla Bot added the cla signed label Jul 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown

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 3 comments.

Comment thread balance/weighting_methods/rake.py
Comment thread balance/weighting_methods/rake.py Outdated
Comment thread balance/weighting_methods/rake.py Outdated
Copilot AI review requested due to automatic review settings July 14, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown

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.

Comment thread balance/weighting_methods/rake.py
Comment thread balance/weighting_methods/rake.py
Comment thread balance/weighting_methods/rake.py Outdated
Comment thread balance/weighting_methods/rake.py Outdated
Comment thread balance/weighting_methods/rake.py Outdated
Comment thread balance/weighting_methods/rake.py Outdated
Comment thread balance/weighting_methods/rake.py Outdated
Comment thread balance/weighting_methods/rake.py
Copilot AI review requested due to automatic review settings July 14, 2026 14:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown

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 5 out of 5 changed files in this pull request and generated 3 comments.

Comment thread balance/weighting_methods/rake.py
Comment thread website/docs/tutorials/quickstart_rake.mdx
Comment thread tutorials/balance_quickstart_rake.ipynb Outdated
Copilot AI review requested due to automatic review settings July 14, 2026 17:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown

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 5 out of 5 changed files in this pull request and generated 3 comments.

Comment on lines +152 to +171
if not isinstance(dict_of_dicts, dict):
raise ValueError(
"dict_of_dicts must be a dictionary of dictionaries; "
f"got {type(dict_of_dicts).__name__}."
)
if not dict_of_dicts:
raise ValueError("dict_of_dicts must be non-empty; got an empty dictionary.")

totals: Dict[str, float] = {}
for var_name, inner_dict in dict_of_dicts.items():
if not isinstance(var_name, str):
raise ValueError(
"Marginal distribution variable names must be strings; "
f"got {type(var_name).__name__}."
)
if not isinstance(inner_dict, dict) or not inner_dict:
raise ValueError(
f"Variable '{var_name}' must map to a non-empty dictionary of "
f"category {value_label}s."
)
Comment thread balance/weighting_methods/rake.py Outdated
Comment on lines 308 to 311
sample_df --- (pandas dataframe) a dataframe representing the sample.
sample_weights --- (pandas series) design weights for sample.
target_df --- (pandas dataframe) a dataframe representing the target.
target_weights --- (pandas series) design weights for target.

@talgalili talgalili left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please update function name + fix other LLM inputs. :)

return totals


def _marginal_category_presence_key(value: Any) -> str:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a general function - with a very specific name.
I propose using a more general name for the function (and again - update the docstring)

Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Copilot AI review requested due to automatic review settings July 15, 2026 08:45
@meta-codesync

meta-codesync Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown

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 5 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +152 to +171
if not isinstance(dict_of_dicts, dict):
raise ValueError(
"dict_of_dicts must be a dictionary of dictionaries; "
f"got {type(dict_of_dicts).__name__}."
)
if not dict_of_dicts:
raise ValueError("dict_of_dicts must be non-empty; got an empty dictionary.")

totals: Dict[str, float] = {}
for var_name, inner_dict in dict_of_dicts.items():
if not isinstance(var_name, str):
raise ValueError(
"Marginal distribution variable names must be strings; "
f"got {type(var_name).__name__}."
)
if not isinstance(inner_dict, dict) or not inner_dict:
raise ValueError(
f"Variable '{var_name}' must map to a non-empty dictionary of "
f"category {value_label}s."
)
Comment on lines 308 to +312
sample_df --- (pandas dataframe) a dataframe representing the sample.
sample_weights --- (pandas series) design weights for sample.
target_df --- (pandas dataframe) a dataframe representing the target.
target_weights --- (pandas series) design weights for target.
target_margins --- (mapping, optional, keyword-only) known target marginal
@talgalili

Copy link
Copy Markdown
Contributor

The PR is in good enough state - I'll take it over to complete some changes - and then land.

@talgalili talgalili closed this Jul 15, 2026
@neuralsorcerer neuralsorcerer deleted the rake branch July 15, 2026 12:02
meta-codesync Bot pushed a commit that referenced this pull request Jul 15, 2026
Summary: Pull Request resolved: #528

Differential Revision: D112085432

Pulled By: talgalili

fbshipit-source-id: f75c4a4959939009314c87dfc8b06fd6e415a404
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.

3 participants