fix(impute): never impute declared id_columns or target_column - #423
Merged
Merged
Conversation
Explicit imputation (impute=/impute_strategy=) filled the declared
id_columns and target_column, contradicting the documented safe default
('never imputes an identifier, modifies a target column'). Only
context-protected columns were skipped.
Skip the declared identifier and target columns in the simple mean/median/
mode strategies too, resolving names after renaming like the protected-column
guard. Report 'skipped: identifier column' / 'skipped: target column' when a
value would have changed, and warn when an impute_strategy entry names one of
them. MissForest already gates these roles with its own audited action, so its
candidate list is unchanged. Defaults are unaffected (impute is off).
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)
…rget # Conflicts: # CHANGELOG.md
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
1 participant
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
Explicit imputation filled the declared
id_columnsandtarget_column, contradicting the documented safe default (README: "never imputes an identifier, modifies a target column"; docs/cleaning-engine.md, trust-claims.md). Only context-protected columns were being skipped.With
impute="mean"/"median"andid_columns=("customer_id",),target_column="churn", both columns had their missing cells filled — corrupting identifiers and leaking into the target. Defaults are unaffected because imputation is off by default.Change
impute_missingnow skips the declaredid_columnsandtarget_columnfor the simplemean/median/mode/autostrategies, in addition to the existing context-protected skip._match_columns(exact, then snake_case), so"Customer ID"still matchescustomer_id.skipped: identifier column/skipped: target columnonly when a value would actually have changed.impute_strategyentry naming one of these columns is ignored with a warning.test_missforest_respects_target_id_text_and_high_missingness_gatespassing.Tests
New cases in
tests/test_missing.py: id/target never imputed across mean/median/mode/auto; animpute_strategyon the target ignored with a warning; a renamed declared id; no skip note when there is nothing to fill; and MissForest never imputing a declared target.Verification
tests/test_missing.py,test_missforest_imputation.py,test_missforest_nullable_int.py,test_impute_column_validation.py,test_engine_missing.py,test_properties.py,test_regressions.py,test_api.py: 145 passed on Python 3.12 / pandas 2.3.3 and Python 3.9 / pandas 1.5.3.ruff check: clean. (mypy runs in CI's quality-fast lane; a local run is blocked by numpy 2.5 stub syntax.)Found by the production-readiness test campaign (lane L5, finding FDC-L5-012). The companion outlier fix (
outliers="clip"clipping the same columns) is handled separately.