[MRG] SimpleImputer skipped feature warning with feature names #21617
Conversation
…nto SimpleImputer-transform-deprecated-improved-message
chritter
changed the title
chritter
changed the title
|
Please clarify if a changelog entry is required. Thanks! |
chritter
changed the title
This PR is a net usability improvement so +1 for merge as it is.
However by reviewing the test, I now realize that I would have expected to get such a warning (or maybe even a ValueError?) at fit time rather than just transform time.
What do you think @chritter and others? If you agree please feel free to create a new issue / follow-up PR once this is merged.
LGTM. Thank you, @chritter.
@ogrisel: I also agree that having a warning or a error raised at fit time would have been better. Yet, might this introduce unwanted and potentially harmful behavior breaking change?
I would be prudent/conservative by keeping the behavior identical but improving the warning explicitness as proposed by this PR.
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
|
Merged! Thanks @chritter. Feel free to open a follow-up PR to raise the warning at fit time instead of transform time. |
…t-learn#21617) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Reference Issues/PRs
Fixes #21616.
What does this implement/fix? Explain your changes.
If feature names are provided SimpleImputer will warn about skipped features with actual feature names. Previously only indices were provided. I introduced an additional test to ensure warning is raised correctly.
Any other comments?
#DataUmbrella Sprint
@reshamas