8000 [MRG] Input validation refactoring by amueller · Pull Request #3443 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Comments

[MRG] Input validation refactoring#3443

Merged
GaelVaroquaux merged 1 commit intoscikit-learn:masterfrom
amueller:input_validation_refactoring
Jul 20, 2014
Merged

[MRG] Input validation refactoring#3443
GaelVaroquaux merged 1 commit intoscikit-learn:masterfrom
amueller:input_validation_refactoring

Conversation

@amueller
Copy link
Member
@amueller amueller commented Jul 19, 2014

Refactor input validation to make it more consistent.

Todo

  • Tests
  • docstrings

@amueller amueller mentioned this pull request Jul 19, 2014
Closed
@amueller
Copy link
Member Author
amueller commented Jul 19, 2014

Hopefully more readable and consistent input checking. Also fixed some corner cases.

@coveralls
Copy link
coveralls commented Jul 19, 2014

Coverage Status

Coverage decreased (-0.01%) when pulling c3ecfed on amueller:input_validation_refactoring into 0807e19 on scikit-learn:master.

arjoly
arjoly reviewed Jul 19, 2014
View reviewed changes
sklearn/utils/validation.py Outdated
Copy link
Member
@arjoly arjoly Jul 19, 2014

Choose a reason for hiding this comment

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

. is not needed

@coveralls
Copy link
coveralls commented Jul 19, 2014

Coverage Status

Coverage decreased (-0.01%) when pulling f72aa17 on amueller:input_validation_refactoring into 0807e19 on scikit-learn:master.

arjoly
arjoly reviewed Jul 19, 2014
View reviewed changes
sklearn/utils/validation.py Outdated
Copy link
Member
@arjoly arjoly Jul 19, 2014

Choose a reason for hiding this comment

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

It assumes that you a .data which is not the case for each sparse matrix.

Copy link
Member Author
@amueller amueller Jul 19, 2014

Choose a reason for hiding this comment

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

that is true. The tests seem not to be so great ^^ we need to add a test for that.

Copy link
Member Author
@amueller amueller Jul 19, 2014

Choose a reason for hiding this comment

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

fixed.

@coveralls
Copy link
coveralls commented Jul 19, 2014

Coverage Status

Coverage decreased (-0.0%) when pulling d03c84e on amueller:input_validation_refactoring into 4ec8630 on scikit-learn:master.

GaelVaroquaux
GaelVaroquaux reviewed Jul 19, 2014
View reviewed changes
sklearn/utils/validation.py Outdated
Copy link
Member
@GaelVaroquaux GaelVaroquaux Jul 19, 2014

Choose a reason for hiding this comment

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

I think that here I would use the 'asformat' method of sparse matrices.

Copy link
Member Author
@amueller amueller Jul 19, 2014

Choose a reason for hiding this comment

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

ahh, I have been looking and not finding this method.

@arjoly
Copy link
Member
arjoly commented Jul 19, 2014

Does dense data always allowed?

@amueller
Copy link
Member Author
amueller commented Jul 19, 2014

Yes, currently dense data is always allowed. Do we have an application where this is not the case? If we do at some point, we could later add a flag?

@coveralls
Copy link
coveralls commented Jul 19, 2014

Coverage Status

Coverage decreased (-0.01%) when pulling 1be9e46 on amueller:input_validation_refactoring into 4ec8630 on scikit-learn:master.

@amueller
Copy link
Member Author
amueller commented Jul 19, 2014

thanks for the helpful reviews guys :)

@amueller amueller changed the title [WIP] Input validation refactoring [MRG] Input validation refactoring Jul 19, 2014
@coveralls
Copy link
coveralls commented Jul 19, 2014

Coverage Status

Coverage increased (+0.01%) when pulling 33f20bf on amueller:input_validation_refactoring into 4ec8630 on scikit-learn:master.

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Jul 19, 2014

I see that this has switched to MRG, but the TODO still lists tests. What's the status on that?

arjoly
arjoly reviewed Jul 19, 2014
View reviewed changes
sklearn/utils/__init__.py
Copy link
Member
@arjoly arjoly Jul 19, 2014

Choose a reason for hiding this comment

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

Can you remove deprecated stuff?

Copy link
Member Author
@amueller amueller Jul 20, 2014

Choose a reason for hiding this comment

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

Not in this PR, but in the next PR which will touch all files.

arjoly
arjoly reviewed Jul 19, 2014
View reviewed changes
sklearn/utils/validation.py Outdated
Copy link
Member
@arjoly arjoly Jul 19, 2014

Choose a reason for hiding this comment

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

order : 'F', 'C' or None (default=None)

@arjoly
Copy link
Member
arjoly commented Jul 19, 2014

Except for the cosmetic comment, 👍

GaelVaroquaux
GaelVaroquaux reviewed Jul 19, 2014
View reviewed changes
sklearn/feature_extraction/image.py
Copy link
Member
@GaelVaroquaux GaelVaroquaux Jul 19, 2014

Choose a reason for hiding this comment

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

Hum, that change is really surprising for me: I would read the 2 lines (the one removed and the one added) as doing very different things. It's probably just a question of choice of names on the arguments.

Copy link
Member Author
@amueller amueller Jul 19, 2014

Choose a reason for hiding this comment

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

that is because the previous behavior was surprising ;)

@coveralls
Copy link
coveralls commented Jul 20, 2014

Coverage Status

Coverage increased (+0.04%) when pulling 95afdc7 on amueller:input_validation_refactoring into 4ec8630 on scikit-learn:master.

@ogrisel
Copy link
Member
ogrisel commented Jul 20, 2014

+1 for merging this PR as it is and deal with further refactoring in a separate PR.

@arjoly
Copy link
Member
arjoly commented Jul 20, 2014

+1

GaelVaroquaux added a commit that referenced this pull request Jul 20, 2014
@GaelVaroquaux
Merge pull request #3443 from amueller/input_validation_refactoring
8dab222 
[MRG] Input validation refactoring
@GaelVaroquaux GaelVaroquaux merged commit 8dab222 into scikit-learn:master Jul 20, 2014
@coveralls
Copy link
coveralls commented Jul 20, 2014

Coverage Status

Coverage increased (+0.01%) when pulling f7549fd on amueller:input_validation_refactoring into 41d02e0 on scikit-learn:master.

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Jul 20, 2014

Merged from the airport!

Awesome work! Go Go team!

@amueller
Copy link
Member Author
amueller commented Jul 20, 2014

err... I just squashed, rebased and merge... hum.... Let's see what'll happen now.

@amueller
Copy link
Member Author
amueller commented Jul 20, 2014

Oh I didn't push. all is good.

@amueller amueller mentioned this pull request Jul 20, 2014
Merged
@amueller amueller mentioned this pull request Jan 16, 2015
Closed
@jakevdp jakevdp mentioned this pull request Jun 22, 2015
Closed
@amueller amueller deleted the input_validation_refactoring branch May 19, 2017 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@amueller @coveralls @arjoly @GaelVaroquaux @ogrisel
0