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

FIX expose fill_value in IterativeImputer #25232

Merged
merged 12 commits into from Jan 17, 2023
Merged

Conversation

thijs-vanweezel
Copy link
Contributor

Reference Issues/PRs

Fixes #25052

What does this implement/fix? Explain your changes.

Adds the parameter fill_value to IterativeImputer.

Any other comments?

@betatim suggested adding support for passing any imputer as initial_strategy. However, I purposefully dismissed this, for the following two reasons:

  • Unforeseeable errors would certainly ensue. For example, if a user were to set add_indicator to True in an imputer passed as initial_strategy, IterativeImputer would probably run into issues.
  • In my view, IterativeImputer could be described as a general imputer, i.e., one that is able to leverage any regressor or classifier for imputation purposes, and thereby making other imputers redundant.

Lastly, additional to the delay resulting from my exams, this was my first time contributing to open source (and in the process, I crashed my PC). Therefore, I am interested to see whether my work meets the sklearn standard.

Cheers.

@glemaitre glemaitre changed the title [MRG] Fixes 'IterativeImputer has no parameter "fill_value" #25052' FIX expose fill_value in IterativeImputer Dec 26, 2022
@glemaitre glemaitre changed the title FIX expose fill_value in IterativeImputer FIX expose fill_value in IterativeImputer Dec 26, 2022
@glemaitre glemaitre self-requested a review December 26, 2022 13:14
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.

We need to add a test to check that we get the proper behaviour.
Since we expose intital_imputer_ we can check that the fill_value is wired to it.

We also need an entry in the v1.3.rst changelog file.

sklearn/impute/_iterative.py Outdated Show resolved Hide resolved
@thijs-vanweezel
Copy link
Contributor Author

@glemaitre Thanks for the feedback. Have your concerns been sufficiently addressed?

@glemaitre glemaitre self-requested a review January 9, 2023 09:23
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, it starts to look good.

sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved
sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved
thijs-vanweezel and others added 3 commits January 9, 2023 11:50
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Tim Head <betatim@gmail.com>
@thijs-vanweezel
Copy link
Contributor Author

@glemaitre, do you have any idea why these tests are failing? I ran pytest and flake8, and neither reported an issue.

@glemaitre
Copy link
Member

I would assume that we need to merge main into your branch. Let me try that.

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.

LGTM. Thanks @ValueInvestorThijs

@glemaitre glemaitre added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jan 14, 2023
@TomDLT TomDLT merged commit 71a647f into scikit-learn:main Jan 17, 2023
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Tim Head <betatim@gmail.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Tim Head <betatim@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:impute Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IterativeImputer has no parameter "fill_value"
4 participants