Repository navigation
fix(missforest): preserve declared roles after header normalization - #526
Conversation
Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
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. |
|
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 |
![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: f47f6584fb
ℹ️ 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".
| for col in missforest_columns: | ||
| resolved_role = roles.get(str(col)) | ||
| if resolved_role is not None: | ||
| contexts[col].role = "id" if resolved_role == "identifier" else "target" |
There was a problem hiding this comment.
When two raw headers such as A B and A-B normalize to a_b and a_b_2, _declared_roles resolves target_column="A-B" to the first snake-case match (a_b). If both columns have missing values and enough rows for modeling, this assignment therefore makes MissForest skip the unrelated first column while the explicitly declared a_b_2 target remains eligible for imputation. Resolve the declaration through the positional raw-to-normalized rename map (or its correct deduplicated label) before overriding the context role.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 25b936a. Exact raw target/id declarations now follow the positional raw-to-normalized mapping before the original header spellings are lost, so A-B correctly resolves to a_b_2. Added six model-backed regression cases covering target, identifier, and overlapping roles with global and per-column MissForest selection; all six failed before this change. An additional control verifies the caller's configuration is unchanged. All 44 focused checks pass on pandas 2.3 and 1.5; the full fast suite has 7,640 passes and only the five reproduced baseline failures.
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! Cleanly fixes #525. Positional raw-to-normalized mapping preserves raw role declarations across header deduplication (A B vs A-B), prevents MissForest model training on protected columns, and preserves config immutability. Ready to merge.
…ared-role-normalization
2 participants
With default header normalization, MissForest could fit and fill a declared target or identifier when its declaration used the original spaced header. Resolve exact raw role declarations through the positional raw-to-normalized mapping, including deduplicated headers such as
A B/A-Bbecominga_b/a_b_2. Carry the resolved roles into MissForest contexts so protected columns retain their values and emit the existing audited skip action. Eligible unprotected features continue to train, and the caller's frozen configuration remains unchanged.Validation: 24 new cases pass, including 13 observed failures before their respective fixes and 11 positive controls. All 44 focused role/model/nullable-integer checks pass on pandas 2.3 and pandas 1.5, including real regressors for eligible features while the declared target or identifier remains untouched. The full fast suite has 7,640 passes, 49 skips, and 94.99% coverage; its five failures match the reproduced Windows baseline (runner timeout, console encoding, generated-code filename escaping, CSV newline round trip, and the zero-budget boundary addressed by #520). Ruff, Linux-targeted mypy across 205 sources, and diff checks pass.
Closes #525