Skip to content

[MRG+2] add validation for non-empty input data #4214

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

Merged
merged 1 commit into from
Feb 14, 2015

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Feb 6, 2015

This follows the discussion in #4206. This makes check_array and check_X_y reject input arrays with less than 1 samples and less than 1 feature for 2D inputs while providing informative error message to the caller.

FYI I also have implemented some common checks based on this PR in edcd079 but this reveals some missing input validation in several estimators. Those missing validation checks should better be added once @amueller's own #4136 is merged in master to avoid conflict resolution pain and redundant work.

@ogrisel ogrisel added this to the 0.16 milestone Feb 6, 2015
@amueller
Copy link
Member

amueller commented Feb 7, 2015

Sounds like a plan :)

check_is_fitted)
NotFittedError,
has_fit_parameter,
check_is_fitted)
Copy link
Member

Choose a reason for hiding this comment

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

STY: One import per line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I did not intend to change those import lines as part of this PR but apparently my editor fixed the indenting without asking me :)

@arjoly
Copy link
Member

arjoly commented Feb 9, 2015

LGTM.

@ogrisel ogrisel changed the title [MRG] add validation for non-empty input data [MRG+1] add validation for non-empty input data Feb 9, 2015
@amueller
Copy link
Member

This needs a common test, right?

@ogrisel
Copy link
Member Author

ogrisel commented Feb 13, 2015

This needs a common test, right?

Yes I started adding some in edcd079 although I think the content of this PR only is already an improvement in its current state. Let me first rebase it on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) to 95.04% when pulling d16e9ee on ogrisel:fix-empty-input-data into cd931fa on scikit-learn:master.

@ogrisel
Copy link
Member Author

ogrisel commented Feb 13, 2015

I added the failing common tests in a separate PR at #4245.

@agramfort
Copy link
Member

LGTM +1 for merge

@agramfort agramfort changed the title [MRG+1] add validation for non-empty input data [MRG+2] add validation for non-empty input data Feb 14, 2015
agramfort added a commit that referenced this pull request Feb 14, 2015
[MRG+2] add validation for non-empty input data
@agramfort agramfort merged commit d0b9955 into scikit-learn:master Feb 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants