Skip to content
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

Jump to bottom

MAINT change default value for min/max of IterativeImputer #16493

Merged
merged 7 commits into from May 12, 2020
Merged

MAINT change default value for min/max of IterativeImputer #16493

merged 7 commits into from May 12, 2020

Conversation

DarshanGowda0
Copy link
Contributor

Reference Issues/PRs

Fixes #16490

What does this implement/fix? Explain your changes.

Changed the default value of max and min to np.inf and -np.inf respectively.

@glemaitre glemaitre changed the title #16490 defualt value for min/max of IterativeImputer MAINT change default value for min/max of IterativeImputer Feb 20, 2020
@glemaitre glemaitre added this to REVIEWED AND WAITING FOR CHANGES in Guillaume's pet Feb 20, 2020
@rth
Copy link
Member

rth commented Feb 20, 2020

Dropping support for min_value=None etc would break backward compatibility though? Should we still support it with a deprecation warning?

@glemaitre
Copy link
Contributor

Dropping support for min_value=None etc would break backward compatibility though? Should we still support it with a deprecation warning?

IterativeImputer is still experimental. In addition, None was the default and it was defaulting on either -inf or +inf which we are replacing it by. I think that we should be fine with both combined reasons. WDYT?

@glemaitre glemaitre self-assigned this Feb 21, 2020
@glemaitre
Copy link
Contributor

@DarshanGowda0 You need to resolve the conflict

@jnothman
Copy link
Member

jnothman commented Feb 22, 2020 via email

@rth
Copy link
Member

rth commented Feb 22, 2020

Yes, definitely, I missed that it was still experimental.

@glemaitre
Copy link
Contributor

I solved the conflicts.

@glemaitre
Copy link
Contributor

@DarshanGowda0

Please add an entry to the change log at doc/whats_new/v0.23.rst under the section sklearn.impute. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:. This change can be considerd as a |Fix|

@DarshanGowda0 DarshanGowda0 mentioned this pull request Mar 2, 2020
Closed
@glemaitre glemaitre self-requested a review May 12, 2020 09:02
@glemaitre
Copy link
Contributor

I added an entry in the what's new. @rth @jnothman Could you have a quick look at this.

glemaitre
glemaitre reviewed May 12, 2020
View reviewed changes
sklearn/impute/_iterative.py
@@ -602,9 +602,9 @@ def fit_transform(self, X, y=None):
self.n_iter_ = 0
return super()._concatenate_indicator(Xt, X_indicator)

self._min_value = IterativeImputer._validate_limit(
self._min_value = self._validate_limit(
Copy link
Contributor

Choose a reason for hiding this comment

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

For reviewer: I made this change just for style because I find this weird.

jnothman
jnothman approved these changes May 12, 2020
View reviewed changes
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.

LGTM

@glemaitre glemaitre merged commit d1f8a16 into scikit-learn:master May 12, 2020
@glemaitre glemaitre moved this from REVIEWED AND WAITING FOR CHANGES to MERGED in Guillaume's pet May 13, 2020
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@DarshanGowda0 @gio8tisu
MAINT change default value for min/max of IterativeImputer (scikit-le…
7592948 
…arn#16493)
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@DarshanGowda0 @viclafargue
MAINT change default value for min/max of IterativeImputer (scikit-le…
be21886 
…arn#16493)
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@DarshanGowda0 @jayzed82
MAINT change default value for min/max of IterativeImputer (scikit-le…
32cac61 
…arn#16493)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@jnothman jnothman jnothman approved these changes

Assignees

@glemaitre glemaitre

Labels
module:impute
Projects
Guillaume's pet
MERGED
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change default valuefor min/max in the experimental IterativeImputer
4 participants
@DarshanGowda0 @rth @glemaitre @jnothman