Skip to content

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

Merged
glemaitre merged 7 commits intoscikit-learn:masterfrom
DarshanGowda0:master
May 12, 2020
Merged

MAINT change default value for min/max of IterativeImputer#16493
glemaitre merged 7 commits intoscikit-learn:masterfrom
DarshanGowda0:master

Conversation

@DarshanGowda0
Copy link
Copy Markdown
Contributor

@DarshanGowda0 DarshanGowda0 commented Feb 20, 2020

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 reviewed Feb 20, 2020
View reviewed changes
@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
@rth
Copy link
Copy Markdown
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
Copy Markdown
Member

glemaitre 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?

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
Copy Markdown
Member

glemaitre commented Feb 21, 2020

@DarshanGowda0 You need to resolve the conflict

glemaitre
glemaitre reviewed Feb 21, 2020
View reviewed changes
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Feb 22, 2020 via email

@rth
Copy link
Copy Markdown
Member

rth commented Feb 22, 2020

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

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Feb 23, 2020

I solved the conflicts.

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Feb 23, 2020

@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
Copy Markdown
Member

glemaitre commented May 12, 2020

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
return super()._concatenate_indicator(Xt, X_indicator)

self._min_value = IterativeImputer._validate_limit(
self._min_value = self._validate_limit(
Copy link
Copy Markdown
Member

@glemaitre glemaitre May 12, 2020

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
Copy Markdown
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
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

None yet

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