fix(impute): reject unknown impute_strategy columns; single fallback record for predictor-less MissForest columns - #373
Merged
Conversation
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
…t fallback for predictor-less columns impute_strategy keys (also produced by Pipeline.impute(columns=)) were matched against str(column) inside the missing-value step, and keys that matched nothing were silently ignored. A pre-rename name such as "Age" after column normalization, or a typo such as "agee", imputed nothing with no error, warning or report action. run_pipeline now validates the keys right after column renaming (and before any drop step, so keys for columns an earlier step later drops remain valid) and raises a ValueError in the same style as duplicate_subset: it lists the unknown keys and the available columns, and when a key is a pre-normalization name it suggests the normalized one. MissForestImputer kept a column with no predictor columns (a one-column frame) in the eligible set. The model loop fell back to simple imputation, then _assign_success recorded the column again as a random-forest regressor/classifier imputation and appended it to columns_imputed twice. Such columns are now filled once by _fallback_fill before scikit-learn is loaded and removed from the eligible set, so the report shows a single missforest_fallback action and scikit-learn is not required when every column falls back. Closes #310 Closes #324
kevincostner17
force-pushed
the
fix/impute-column-validation
branch
from
September 15, 2026 09:18
c33a011 to
7c91790
Compare
This was referenced Sep 15, 2026
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.
Summary
impute_strategykeys (including those produced byPipeline.impute(columns=)) that match no column were silently ignored: a pre-rename name like"Age"afternormalize_columns(), or a typo like"agee", imputed nothing with no error, warning or report action.run_pipelinenow validates the keys right after column renaming and raises aValueErrorin the same style asduplicate_subset: it lists the unknown keys and available columns and, when a key is a pre-normalization name, suggests the normalized one ('Age' -> 'age'). Validation runs before any drop step, so keys for columns a later step removes are still accepted, and before any mutation of the input.columns_imputedtwice. Such columns are now filled once by the simple fallback before scikit-learn is loaded and removed from the model loop, so the report shows a singlemissforest_fallbackaction and scikit-learn is not required when every column falls back.Behaviour change
Configs with
impute_strategykeys that name no column (after renaming) now raise instead of being silently ignored. This includes a reused config across frames where the column is missing from some of them.Tests
New
tests/test_impute_column_validation.py:fd.cleantypo, default renaming with the original name, unknown key without renaming (no rename note), normalized name imputes, original name imputes when renaming is off, key for a column later dropped bydrop_empty_columnsis accepted, non-string labels matched by string key, input untouched on error.columns_imputedentry; same with scikit-learn imports blocked, via bothimpute="missforest"andimpute_strategy={"x": "missforest"}; frames with predictors still fit a regressor exactly once.Verification
ruff check .— cleanmypy src/freshdata— no issues (202 files)pytest -m "not online and not large"— py3.12: 4258 passed, 6 skipped; py3.9: 4254 passed, 10 skippedCloses #310
Closes #324