Validate outcome model sample weights#548
Conversation
There was a problem hiding this comment.
⚠️ Not ready to approve
One newly introduced ValueError message uses inconsistent parameter/input names (sample_weights/sample_df) relative to the fit_outcome_model API, reducing how actionable the validation error is for callers.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
There was a problem hiding this comment.
⚠️ Not ready to approve
One validation path still produces confusing error text (referring to “sample_df/sample_weights”) and the new elementwise type-check loop can be cheaply optimized to avoid Python iteration for numeric dtypes.
Review details
Comments suppressed due to low confidence (1)
balance/outcome_models/outcome_model.py:449
- _check_weighting_methods_input is called with object_name="sample", so length/index mismatch errors mention "sample_df" / "sample_weights" even though this API uses covars_df + sample_weight; that makes the ValueError less actionable and contradicts the changelog’s wording.
_check_weighting_methods_input(covars_df, sample_weight_series, "sample")
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
There was a problem hiding this comment.
⚠️ Not ready to approve
A misleading error-message path can still surface (sample_df/sample_weights) due to _check_weighting_methods_input(..., object_name="sample"), undermining the PR’s goal of actionable validation errors.
Review details
Comments suppressed due to low confidence (1)
balance/outcome_models/outcome_model.py:450
_prepare_sample_weightrelies on_check_weighting_methods_input(..., object_name="sample"), which can raise errors mentioningsample_df/sample_weightson length/index mismatches. In the outcome-model API the parameter issample_weightand the reference frame iscovars_df, so those messages are misleading and inconsistent with the otherValueErrors in this function (which correctly mentionsample_weight/covars_df). Consider validating length/index here with outcome-model-specific, actionable messages and dropping the_check_weighting_methods_inputcall.
_check_weighting_methods_input(covars_df, sample_weight_series, "sample")
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
|
@neuralsorcerer is the latest response by copilot correct? WDYT? |
Not required. We can proceed to merge. |
|
@talgalili has imported this pull request. If you are a Meta employee, you can view this in D113763728. |
|
@neuralsorcerer has updated the pull request. You must reimport the pull request before landing. |
Please re-execute the CI, it should pass. |
No description provided.