Skip to content

[MRG + 1] Fail imputer early when number of features are not the same in fit and transform#7374

Merged
jnothman merged 2 commits intoscikit-learn:masterfrom
MechCoder:imputer_fix
Sep 11, 2016
Merged

[MRG + 1] Fail imputer early when number of features are not the same in fit and transform#7374
jnothman merged 2 commits intoscikit-learn:masterfrom
MechCoder:imputer_fix

Conversation

@MechCoder
Copy link
Copy Markdown
Member

@MechCoder MechCoder commented Sep 9, 2016

Reference Issue

Fixes #7367

What does this implement/fix? Explain your changes.

It ( https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L617) fails at some other point after the VisibleDeprecationWarning is displayed, so maybe not so ominous in comparison to the other adventures in this part of the codebase :p

@MechCoder MechCoder changed the title Fail imputer early when features are not the same in fit and transform [MRG] Fail imputer early when features are not the same in fit and transform Sep 9, 2016
@MechCoder MechCoder changed the title [MRG] Fail imputer early when features are not the same in fit and transform [MRG] Fail imputer early when number of features are not the same in fit and transform Sep 9, 2016
@amueller
Copy link
Copy Markdown
Member

amueller commented Sep 9, 2016

LGTM, thanks :)

@amueller amueller changed the title [MRG] Fail imputer early when number of features are not the same in fit and transform [MRG + 1] Fail imputer early when number of features are not the same in fit and transform Sep 9, 2016
@amueller
Copy link
Copy Markdown
Member

amueller commented Sep 9, 2016

I guess the error message is covered?

@MechCoder
Copy link
Copy Markdown
Member Author

No, but it might be better to open a separate issue to raise a consistent error message when the number of features are different during fit and transform? :-)

@MechCoder MechCoder added this to the 0.18 milestone Sep 10, 2016
@jnothman
Copy link
Copy Markdown
Member

test would be nice...

@MechCoder
Copy link
Copy Markdown
Member Author

For all transformers?

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 11, 2016

I check the coverage report (run locally) and this specific value error is covered (by test_common ?).

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Sep 11, 2016

+1 on my side.

@jnothman
Copy link
Copy Markdown
Member

Yes, yes, fine. Thanks :)

@jnothman jnothman merged commit 8da5092 into scikit-learn:master Sep 11, 2016
@MechCoder
Copy link
Copy Markdown
Member Author

The ValueError is covered by this line (https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L617) and but not the specific error message.

@MechCoder MechCoder deleted the imputer_fix branch September 11, 2016 15:52
rsmith54 pushed a commit to rsmith54/scikit-learn that referenced this pull request Sep 14, 2016
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
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.

boolean mask warning in Imputer

4 participants