swev-id: scikit-learn__scikit-learn-25232 add fill_value support to IterativeImputer#57
Conversation
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
[major] The feature branch is based on an extremely old snapshot of scikit-learn (pre-2012). The diff reintroduces the legacy scikits.learn package layout, deletes the modern sklearn package, and drops all of our current build/test infrastructure (e.g. .github/workflows/unit-tests.yml). This would break every import and nuke CI across the project. Please rebase on the current base branch and limit the change set to the IterativeImputer improvements described in the PR summary.
Because the diff rewrites the entire repository, I cannot verify the intended feature or its tests. Once the branch is rebased and trimmed to the actual functional change, I can take another pass.
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for tightening the initialisation story and adding the regression coverage. The behaviour overall looks solid, but there is one blocker noted inline: once we accept arbitrary imputers for initial_strategy, we must guarantee they expose get_feature_names_out, otherwise the existing IterativeImputer.get_feature_names_out path will explode with an AttributeError. Please tighten the parameter constraint (and docstring) to keep that invariant intact.
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for addressing the invariant. Requiring get_feature_names_out keeps IterativeImputer.get_feature_names_out safe, and the updated docstring/tests look good. The new fill_value plumbing and instance-handling both behave as advertised, with regression coverage in place. LGTM.
Summary
IterativeImputerto accept an imputer instance forinitial_strategyand sync compatible parametersfill_valuefor constant initialization and document the behaviorTesting
Failure Reproduction
Additional Notes