Skip to content

[MRG] Initialize MissingIndicator with error_on_new = False #13974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jun 24, 2019

Conversation

fhoang7
Copy link
Contributor

@fhoang7 fhoang7 commented May 29, 2019

Reference Issues/PRs

Resolves #13968

What does this implement/fix? Explain your changes.

Set error_on_new to False by default if add_indicator is True in order to suppress missing value errors in ignored features.

Any other comments?

@fhoang7 fhoang7 changed the title [WIP] Initialize MissingIndicator with error_on_new = False [MRG] Initialize MissingIndicator with error_on_new = False May 29, 2019
@jnothman
Copy link
Member

Please add a non-regression test. Thanks.

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume this also applies to IterativeImputer

@fhoang7
Copy link
Contributor Author

fhoang7 commented May 29, 2019

Would the test be to ensure that errors are thrown if relevant features are missing values?

@jnothman
Copy link
Member

jnothman commented May 29, 2019 via email

@fhoang7 fhoang7 changed the title [MRG] Initialize MissingIndicator with error_on_new = False [WIP] Initialize MissingIndicator with error_on_new = False May 30, 2019
…e IterativeImputer nonregression test and also default error_on_new to False.
@fhoang7 fhoang7 changed the title [WIP] Initialize MissingIndicator with error_on_new = False [MRG] Initialize MissingIndicator with error_on_new = False May 30, 2019
@fhoang7
Copy link
Contributor Author

fhoang7 commented Jun 1, 2019

@jnothman can you review? Thanks!

@thomasjpfan
Copy link
Member

Please revert the changes that are not related to the PR.

@fhoang7
Copy link
Contributor Author

fhoang7 commented Jun 1, 2019

Done! @thomasjpfan

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an entry to the change log at doc/whats_new/v0.21.rst under the 0.21.3 change log heading. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:

@fhoang7
Copy link
Contributor Author

fhoang7 commented Jun 2, 2019

Done! @jnothman. Thanks for the help/input.

@fhoang7
Copy link
Contributor Author

fhoang7 commented Jun 3, 2019

@thomasjpfan: change being pushed up.

@jnothman
Copy link
Member

jnothman commented Jun 3, 2019

@NicolasHug I proposed putting this is an 0.21 series bug fix release, assuming we have a couple more fixes to join if (not sure if you want the recently raised divide by zero in gradient boosting to be fixed in 0.21.3 also, for instance)

@amueller
Copy link
Member

Agree this should be a bug-fix release.

@fhoang7
Copy link
Contributor Author

fhoang7 commented Jun 12, 2019

Implemented suggestions @NicolasHug @amueller

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

@thomasjpfan thomasjpfan added this to the 0.21.3 milestone Jun 24, 2019
@thomasjpfan thomasjpfan merged commit 7f50e82 into scikit-learn:master Jun 24, 2019
@thomasjpfan
Copy link
Member

Thank you @fhoang7!

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Jun 25, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design of add_indicator in SimpleImputer may fail when running cross validation
6 participants