Fix misaligned sample_weight in group holdout split - #1586
Open
Jeremy Schoemaker (shoemoney) wants to merge 2 commits into
Open
Fix misaligned sample_weight in group holdout split#1586Jeremy Schoemaker (shoemoney) wants to merge 2 commits into
Jeremy Schoemaker (shoemoney) wants to merge 2 commits into
Conversation
… vector With split_type="group", eval_method="holdout", and sample_weight set, the group branch of _prepare_data splits X, y, and groups by train_idx/val_idx but leaves state.fit_kwargs["sample_weight"] at full length and never sets state.weight_val. AutoMLState.prepare_sample_train_data then slices the weights positionally (weight[:sample_size]), so every trial trains with weights belonging to other rows, and the validation loss that drives model selection ignores sample weights entirely. No error is raised. The "time" branch (generic_task.py:913-955) and _train_test_split (generic_task.py:309) already split the weights this way; the group branch is the only holdout branch that does not. This mirrors that pattern inside the gss loop: slice the weights by train_idx/val_idx into state.fit_kwargs["sample_weight"] and state.weight_val, handling both pd.Series and array inputs. The regression test spies on RandomForestClassifier.fit and encodes each row's expected weight in the row itself, so any misalignment between the rows an estimator receives and the weights it receives fails the assert.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes sample-weight alignment for group-based holdout evaluation.
Changes:
- Splits training and validation weights using group split indices.
- Adds a regression test verifying estimator weight alignment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
flaml/automl/task/generic_task.py |
Splits group holdout sample weights. |
test/automl/test_split.py |
Tests weight alignment and validation weights. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+967
to
+975
| if "sample_weight" in state.fit_kwargs: | ||
| # NOTE: _prepare_data is before kwargs is updated to fit_kwargs_by_estimator | ||
| weight = state.fit_kwargs["sample_weight"] | ||
| if isinstance(weight, pd.Series): | ||
| state.fit_kwargs["sample_weight"] = weight.iloc[train_idx] | ||
| state.weight_val = weight.iloc[val_idx] | ||
| else: | ||
| state.fit_kwargs["sample_weight"] = weight[train_idx] | ||
| state.weight_val = weight[val_idx] |
| X = np.column_stack([np.arange(n, dtype=float), rng.normal(size=n)]) | ||
| groups = np.repeat(np.arange(20), 10) | ||
| y = (groups % 2).astype(int) | ||
| sample_weight = 1000.0 + np.arange(n, dtype=float) |
Convert non-Series sample_weight to ndarray before indexing by train_idx/ val_idx in the group holdout branch, so a plain list or tuple weight does not raise TypeError on fancy indexing, and skip splitting when weight is None. Add a regression test that uses a pd.Series with a non-default index to confirm the split stays positional (.iloc), mirroring the existing ndarray-only test.
Author
|
Done in ddd01f1: the else-branch now converts weight to ndarray before indexing (so a list/tuple no longer raises TypeError) and skips splitting when weight is None; added a second regression test using a pd.Series with a non-default index to confirm the split stays positional. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
With
split_type="group",eval_method="holdout", andsample_weightset, the group branch of_prepare_data(flaml/automl/task/generic_task.py:956-966 on main) splits X, y, and groups bytrain_idx/val_idxbut leavesstate.fit_kwargs["sample_weight"]at full length and never setsstate.weight_val.AutoMLState.prepare_sample_train_data(flaml/automl/state.py:241) then slices the weights positionally,weight[:sample_size], so every trial trains with weights that belong to other rows: rows after the first held-out group get a neighbor's weight, and the held-out rows' weights land on training rows. Becauseweight_valstaysNone, the validation loss that drives model selection ignores sample weights entirely. All of this is silent, no error is raised, and the wrong model can be selected.#1554 fixed the crash in this configuration (#887, #1553) by chaining the
"group"branch as anelifand settingstate.sample_weight_all. It did not change how the weights are split. The pattern this PR follows is the pre-existing one: the"time"branch (generic_task.py:913-955) and_train_test_split(generic_task.py:309) both split weights intostate.fit_kwargs["sample_weight"]andstate.weight_val. The"group"branch is the only holdout branch that does not. This PR mirrors that pattern inside theGroupShuffleSplitloop, handling bothpd.Seriesand array weights.The regression test encodes each row's expected weight in the row itself (column 0 is a row id, weight is 1000 + id) and spies on
RandomForestClassifier.fit, so any misalignment between the rows an estimator receives and the weights it receives fails the assert; it also assertsweight_valis populated. Without the source change the test fails on the alignment assert; with it,test/automl/test_split.pypasses 9/9.Not covered here: the group holdout path still does not handle Spark dataframes (the time branch has an explicit
_split_pysparkarm, the group branch never did, andGroupShuffleSplitcannot split a psDataFrame). That is a pre-existing limitation, and this PR keeps the same pandas/numpy scope as the bug.Related issue number
Follow-up to #1554 (issues #887, #1553), which stopped the crash in this configuration but left the group branch's weights unsplit.
Checks