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

[MRG] Fix LassoCV cross validation split() call #8973

Merged
merged 3 commits into from Jun 7, 2017

Conversation

@paulochf
Copy link
Contributor

@paulochf paulochf commented Jun 1, 2017

Reference Issue

Fixes #8426

What does this implement/fix? Explain your changes.

Added the missing y parameter that was causing the error TypeError: split() missing 1 required positional argument: 'y', and its non-regression test.

Any other comments?

Will be positively influenced by issue #8971.

@paulochf
Copy link
Contributor Author

@paulochf paulochf commented Jun 1, 2017

This test failed, but it seems is not related to this PR.


pipe = make_pipeline(
StandardScaler(),
LassoCV(cv=KFold(n_splits=5))

This comment has been minimized.

@agramfort

agramfort Jun 2, 2017
Member

this test would fail on master?

This comment has been minimized.

@raghavrv

raghavrv Jun 2, 2017
Member

it should be StratifiedKFold...

This comment has been minimized.

@paulochf

paulochf Jun 2, 2017
Author Contributor

Sorry, @raghavrv! My bad at pasting.

Fixed that, although I could not run tests locally today.

Copy link
Member

@raghavrv raghavrv left a comment

Small change in the tests... Otherwise good for merge


pipe = make_pipeline(
StandardScaler(),
LassoCV(cv=KFold(n_splits=5))

This comment has been minimized.

@raghavrv

raghavrv Jun 2, 2017
Member

it should be StratifiedKFold...

def test_lasso_cv_with_some_model_selection():
from sklearn.pipeline import make_pipeline
from sklearn.preprocessing import StandardScaler
from sklearn.model_selection import KFold

This comment has been minimized.

@raghavrv

raghavrv Jun 2, 2017
Member

StratifiedKFold

@jnothman
Copy link
Member

@jnothman jnothman commented Jun 7, 2017

LGTM, thanks

@jnothman jnothman merged commit 0e4bdfd into scikit-learn:master Jun 7, 2017
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 94.88%)
Details
codecov/project 94.88% (+<.01%) compared to 7ce7134
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
@paulochf
Copy link
Contributor Author

@paulochf paulochf commented Jun 14, 2017

Thank you for your help! Learnt a lot doing this, and happy for detecting the problem issue 8971 is going to solve.

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
* Fixing cross validation split call in LassoCV

* Non-regression test for LassoCV cv.split check

* Fix typo KFold->StratifiedKFold
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.