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

Check arguments of `MissingIndicator` imputer when handling sparse arrays #13240

Merged
merged 15 commits into from Feb 26, 2019

Conversation

5 participants
@btel
Copy link
Contributor

btel commented Feb 25, 2019

Reference Issues/PRs

Fixes issue #12750

What does this implement/fix? Explain your changes.

This PR implements a simple check of the arguments to make sure that the MissingIndicator raises an exception when both arguments sparse=True and missing_values=0 or array is sparse and missing_values=0. It fixes old tests that allowed for these cases and implements one new test.

Any other comments?

Show resolved Hide resolved sklearn/impute.py Outdated
Bartosz Telenczuk
@glemaitre
Copy link
Contributor

glemaitre left a comment

Could you add an entry in what's new

Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated
Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated
Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py Outdated

Bartosz Telenczuk added some commits Feb 25, 2019

@btel

This comment has been minimized.

Copy link
Contributor Author

btel commented Feb 25, 2019

ok. I fixed all the issues that came up in the review. Most importantly,

  • removed spourius blank lines
  • removed the check for arguments sparse=True and missing_values=0, which, as pointed out by @jeremiedbb , was not necessary
  • added check in transform method
@jeremiedbb

This comment has been minimized.

Copy link
Contributor

jeremiedbb commented Feb 25, 2019

I think it's fine now. But maybe it would be more readable to separate the case sparse input + missing = 0, i.e. have a test which checks everything works on normal inputs and another one which checks that an error is raised otherwise.

@btel

This comment has been minimized.

Copy link
Contributor Author

btel commented Feb 25, 2019

@jeremiedbb that makes sense, but I would need to skip over some input combinations. Otherwise, I couldn't use pytest decorators. How would you do that?

@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Feb 25, 2019

@jeremiedbb

This comment has been minimized.

Copy link
Contributor

jeremiedbb commented Feb 25, 2019

I would remove the missing=0 case from the current test and add a test with missing=0.

@btel

This comment has been minimized.

Copy link
Contributor Author

btel commented Feb 25, 2019

ok. I enumerated all possible combinations not involving missing_values == 0 and sparse inputs. For the latter case, I added a new test.

Bartosz Telenczuk added some commits Feb 25, 2019

Bartosz Telenczuk
Bartosz Telenczuk
Bartosz Telenczuk
Show resolved Hide resolved doc/whats_new/v0.21.rst Outdated
@@ -167,7 +171,7 @@ Support for Python 3.4 and below has been officially dropped.

- |Fix| Fixed a bug in :class:`linear_model.LassoLarsIC`, where user input
``copy_X=False`` at instance creation would be overridden by default
parameter value ``copy_X=True`` in ``fit``.
parameter value ``copy_X=True`` in ``fit``.

This comment has been minimized.

Copy link
@jeremiedbb

jeremiedbb Feb 26, 2019

Contributor

Please avoid fixing pep8 issues not related to your PR. It makes reviews harder to follow

This comment has been minimized.

Copy link
@btel

btel Feb 26, 2019

Author Contributor

Ok. I will avoid it for future PRs, but I guess I should keep this one.

This comment has been minimized.

Copy link
@jeremiedbb

jeremiedbb Feb 26, 2019

Contributor

we usually ask contributors to revert it :/

Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/impute.py Outdated
Show resolved Hide resolved sklearn/tests/test_impute.py

@GaelVaroquaux GaelVaroquaux added this to To do in Sprint Paris 2019 via automation Feb 26, 2019

@GaelVaroquaux GaelVaroquaux moved this from To do to Needs review in Sprint Paris 2019 Feb 26, 2019

Bartosz Telenczuk added some commits Feb 26, 2019

@jeremiedbb
Copy link
Contributor

jeremiedbb left a comment

LGTM

@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Feb 26, 2019

LGTM. Will merged when CI are green.

@agramfort agramfort merged commit 5890cbc into scikit-learn:master Feb 26, 2019

11 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 92.49%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +7.5% compared to f8b108d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Sprint Paris 2019 automation moved this from Needs review to Done Feb 26, 2019

@agramfort

This comment has been minimized.

Copy link
Member

agramfort commented Feb 26, 2019

thx @btel

@btel btel deleted the btel:fix_missing_imputer branch Feb 26, 2019

@btel

This comment has been minimized.

Copy link
Contributor Author

btel commented Feb 26, 2019

my first every scikit-learn PR merged 🎊 Thanks @jeremiedbb @glemaitre @jnothman @agramfort for help!

@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Feb 26, 2019

🍺 Thanks a lot.

Kiku-git added a commit to Kiku-git/scikit-learn that referenced this pull request Mar 4, 2019

Check arguments of `MissingIndicator` imputer when handling sparse ar…
…rays (scikit-learn#13240)

* adding argument check for the case spare=True and missing_values=0
issue scikit-learn#12750

* missing indicator: handle case array is sparse and missing values==0
issue scikit-learn#12750

* fix flake8 issues

* remove empty lines

* remove sparse==True and missing_value==0 case; handle `transform` method

* remove blankline

* separate out test for missing_value==0 and sparse input

* remove spurious blank lines

* added what's new entry

* fixing lint issues

* remove redundant code

* fix issue number in "what's new"

* move duplicate code to _validate_input

* fix test for transform method

* remove nested if conditions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.