Repository navigation
docs(cookbook): choose and audit imputation strategies - #523
JohnnyWilson16 merged 2 commits into
Conversation
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b1c0a60c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Missing ML dependencies, insufficient predictors, and ineligible columns can | ||
| also lead to fallback or preservation; inspect the audit rather than assuming | ||
| a requested model ran. Declared IDs and targets are not imputed, but role |
There was a problem hiding this comment.
When column_names=True renames a declared target (for example Future Sales to future_sales) and the target name is not one of the built-in target heuristics, this assurance does not hold for missforest. impute_missing() snake-matches declared roles only for simple imputation; the MissForest branch instead calls build_contexts(), and infer_role() compares the already-normalized label to config.target_column/id_columns exactly, so an eligible missing target can be fitted and filled. Normalize those declarations for the MissForest gate (or document the required post-normalization names) before telling users the label is protected, since this can introduce target leakage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated in 4974db7. Reproduced: declaring the original Future Sales header allows eight missing target cells to be fitted, while declaring future_sales preserves all eight. The guide now requires post-normalization names for both role declarations and imputation rules, explains the MissForest exact-match gate, and includes a runnable custom-target preservation example. All five code blocks pass on pandas 1.5 and 2.3.
| assert "fewer rows" in fallback_actions[0].metadata["fallback_reason"] | ||
| ``` | ||
|
|
||
| Missing ML dependencies, insufficient predictors, and ineligible columns can |
There was a problem hiding this comment.
When scikit-learn is absent and a requested MissForest column is otherwise model-eligible (as in the 80-row example), MissForestImputer.impute() calls _sklearn_forests(), which raises ImportError; it does not return a fallback/preservation action or any report to inspect. Separate this failure case from the audited data-eligibility fallbacks so readers do not expect cleaning to continue without the [ml] extra.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated in 4974db7. Corrected the model-dependency case separately from data-eligibility fallbacks. For model-eligible data, missing scikit-learn raises ImportError and returns no report. I verified that behavior by blocking sklearn imports during the actual cleaning call; the guide now tells readers to install the ML extra before retrying.
| | --- | --- | | ||
| | `strategy="conservative"`, `impute=None` | Representation repairs; no decision-engine imputation. | | ||
| | `strategy="balanced"`, `impute=None` | Accuracy-first decisions based on column roles, missingness, and available evidence. Gaps may remain. | | ||
| | `strategy="aggressive"`, `impute=None` | More active filling, column drops, and capping where the engine's guards allow them. | |
There was a problem hiding this comment.
With exactly the documented strategy="aggressive", impute=None combination and default options, outlier_action is still "auto"; select_outlier_action() flags outliers for both strategies and explicitly never caps them. Capping occurs only when outlier_action="cap" is requested, so this row currently leads readers to expect value rewriting that will not occur.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated in 4974db7. Corrected the aggressive-strategy row to describe default outlier flagging and require outlier_action="cap" for capping. This matches select_outlier_action; the strict documentation build and both example compatibility runs pass.
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
JohnnyWilson16
left a comment
JohnnyWilson16
left a comment
There was a problem hiding this comment.
LGTM! Excellent cookbook addition for #513. All five code blocks execute deterministically, model and fallback metadata are accurately explained, and strict MkDocs validation passes. Ready to merge.
2 participants
Add the requested imputation-strategies cookbook and link it from the Cookbook index and navigation. It compares mean/median/mode with the MissForest-inspired model path, explains global versus per-column overrides under conservative/balanced/aggressive strategies, and demonstrates declared ID/target protection using post-normalization role names.
Runnable examples inspect aggregate action counts and use an aligned before/after comparison to identify the exact filled cells. The guide distinguishes report granularity from a cell history, exercises an actual random-forest regressor, and shows the small-data fallback and its recorded reason. It also removes the unused PlanGenerator import noted in the review of #522.
Validation: all five Python code blocks were executed directly from the Markdown on pandas 2.3 and pandas 1.5. Assertions verify exact repair coordinates and values, protected columns, simple global strategies, real model execution and metadata, fallback behavior, and the ImportError raised when model-eligible data lacks scikit-learn. The strategy table distinguishes default flagging from explicitly requested capping.
mkdocs build --strictandgit diff --checkpass.Closes #513