[MRG] BUG: check equality instead of identity in check_cv by glemaitre · Pull Request #12155 · scikit-learn/scikit-learn

glemaitre · 2018-09-25T08:50:08Z

Check for equality instead of identity which does not hold when n_jobs=-1 due to some copy.

glemaitre · 2018-09-25T08:51:54Z

I did not put any entry in what's new since that we introduce it in 0.20 and that we should add it inside the upcoming release.

Need to be back-ported to #11948

glemaitre · 2018-09-25T08:53:03Z

ping @jnothman @jorisvandenbossche @qinhanmin2014 @rth @ogrisel

qinhanmin2014

LGTM, thanks for the PR and sorry for the bug.

qinhanmin2014 · 2018-09-25T09:18:36Z

If this is included in 0.20.1, maybe we need an entry. But I prefer to hurry it into 0.20 since it seems to be serious.

rth

Thanks for catching this!

There are a few other similar cases there that might need to be fixed,

$ rg "\w+ is '"
model_selection/_split.py
425:        if n_splits is 'warn':
496:        if n_splits is 'warn':
597:        if n_splits is 'warn':
751:        if n_splits is 'warn':
1942:    if cv is None or cv is 'warn':

rth · 2018-09-25T09:19:37Z

sklearn/model_selection/tests/test_validation.py

+    # the parameters leading to a failure in check_cv due to cv is 'warn'
+    # instead of cv == 'warn'.
+    X, y = load_iris(return_X_y=True)
+    clf = SVC(gamma='auto')


Can we just use a LogisticRegresion? (Trying to gain ms on the test suite run, also just makes sense to use the simplest estimator there is since it's not relevant)

rth · 2018-09-25T09:20:23Z

sklearn/model_selection/tests/test_validation.py

+    X, y = load_iris(return_X_y=True)
+    clf = SVC(gamma='auto')
+    grid = GridSearchCV(clf, param_grid={'C': [1, 10]})
+    cross_validate(grid, X, y, n_jobs=-1)


n_jobs=2 should be enough?

glemaitre · 2018-09-25T09:22:26Z

sorry for the bug

Don't worry I think that this one of the side of sprinting :)

jorisvandenbossche · 2018-09-25T09:30:36Z

+1 on including this in 0.20.0 (so re-doing the tag)

qinhanmin2014 · 2018-09-25T09:30:36Z

More generally, maybe we should avoid comparing strings with is in the repo.

rth · 2018-09-25T09:34:05Z

More generally, maybe we should avoid comparing strings with is in the repo.

Haven't seen other occurrences, but I may have missed something (e.g. when a = 'something' and comparing b is a, but I don't think it's a typical pattern).

glemaitre · 2018-09-25T09:37:20Z

We cannot tests n_splits is 'warns' because it will be modify in __init__ which is make it works. However, I think that it is much better to change it.

rth · 2018-09-25T09:39:16Z

+1 for fixing it even without tests..

qinhanmin2014 · 2018-09-25T09:40:01Z

I'm fine with changing is to == without tests since is is not stable.
I'm not with my PC now and will do a git grep in the repo soon.

qinhanmin2014

LGTM. Let's merge when green.

ogrisel · 2018-09-25T11:29:37Z

I am ok with redoing the tag as well to include this as long as nothing has been pushed to pypi yet.

* tag '0.20.0': (77 commits) ColumnTransformer generalization to work on empty lists (scikit-learn#12084) add sparse_threshold to make_columns_transformer (scikit-learn#12152) [MRG] Convert ColumnTransformer input list to numpy array (scikit-learn#12104) Change version to 0.20.0 BUG: check equality instead of identity in check_cv (scikit-learn#12155) [MRG] Fix FutureWarnings in logistic regression examples (scikit-learn#12114) [MRG] Update test_metaestimators to pass y parameter when calling score (scikit-learn#12089) DOC Removed duplicated doc in tree.rst (scikit-learn#11922) [MRG] DOC covariance doctest examples (scikit-learn#12124) typo and formatting fixes in 0.20 doc (scikit-learn#11963) DOC Replaced the deprecated early_stopping parameter with n_iter_no_change. (scikit-learn#12133) [MRG +1] ColumnTransformer: store evaluated function column specifier during fit (scikit-learn#12107) Fix typo (scikit-learn#12126) DOC Typo in OneHotEncoder DOC Update fit_transform docstring of OneHotEncoder (scikit-learn#12117) DOC Removing quotes from variant names. (scikit-learn#12113) DOC BaggingRegressor missing default value for oob_score in docstring (scikit-learn#12108) [MRG] MNT Re-enable PyPy CI (scikit-learn#12039) MNT Only checks warnings on latest depedendencies versions in CI (scikit-learn#12048) TST Ignore warnings in common test to avoid collection errors (scikit-learn#12093) ...

* releases: (77 commits) ColumnTransformer generalization to work on empty lists (scikit-learn#12084) add sparse_threshold to make_columns_transformer (scikit-learn#12152) [MRG] Convert ColumnTransformer input list to numpy array (scikit-learn#12104) Change version to 0.20.0 BUG: check equality instead of identity in check_cv (scikit-learn#12155) [MRG] Fix FutureWarnings in logistic regression examples (scikit-learn#12114) [MRG] Update test_metaestimators to pass y parameter when calling score (scikit-learn#12089) DOC Removed duplicated doc in tree.rst (scikit-learn#11922) [MRG] DOC covariance doctest examples (scikit-learn#12124) typo and formatting fixes in 0.20 doc (scikit-learn#11963) DOC Replaced the deprecated early_stopping parameter with n_iter_no_change. (scikit-learn#12133) [MRG +1] ColumnTransformer: store evaluated function column specifier during fit (scikit-learn#12107) Fix typo (scikit-learn#12126) DOC Typo in OneHotEncoder DOC Update fit_transform docstring of OneHotEncoder (scikit-learn#12117) DOC Removing quotes from variant names. (scikit-learn#12113) DOC BaggingRegressor missing default value for oob_score in docstring (scikit-learn#12108) [MRG] MNT Re-enable PyPy CI (scikit-learn#12039) MNT Only checks warnings on latest depedendencies versions in CI (scikit-learn#12048) TST Ignore warnings in common test to avoid collection errors (scikit-learn#12093) ...

* dfsg: (77 commits) ColumnTransformer generalization to work on empty lists (scikit-learn#12084) add sparse_threshold to make_columns_transformer (scikit-learn#12152) [MRG] Convert ColumnTransformer input list to numpy array (scikit-learn#12104) Change version to 0.20.0 BUG: check equality instead of identity in check_cv (scikit-learn#12155) [MRG] Fix FutureWarnings in logistic regression examples (scikit-learn#12114) [MRG] Update test_metaestimators to pass y parameter when calling score (scikit-learn#12089) DOC Removed duplicated doc in tree.rst (scikit-learn#11922) [MRG] DOC covariance doctest examples (scikit-learn#12124) typo and formatting fixes in 0.20 doc (scikit-learn#11963) DOC Replaced the deprecated early_stopping parameter with n_iter_no_change. (scikit-learn#12133) [MRG +1] ColumnTransformer: store evaluated function column specifier during fit (scikit-learn#12107) Fix typo (scikit-learn#12126) DOC Typo in OneHotEncoder DOC Update fit_transform docstring of OneHotEncoder (scikit-learn#12117) DOC Removing quotes from variant names. (scikit-learn#12113) DOC BaggingRegressor missing default value for oob_score in docstring (scikit-learn#12108) [MRG] MNT Re-enable PyPy CI (scikit-learn#12039) MNT Only checks warnings on latest depedendencies versions in CI (scikit-learn#12048) TST Ignore warnings in common test to avoid collection errors (scikit-learn#12093) ...

BUG: check equality instead of identity in check_cv

aeaed79

glemaitre changed the title ~~BUG: check equality instead of identity in check_cv~~ [MRG] BUG: check equality instead of identity in check_cv Sep 25, 2018

glemaitre added the Blocker label Sep 25, 2018

glemaitre added this to the 0.20 milestone Sep 25, 2018

jorisvandenbossche mentioned this pull request Sep 25, 2018

0.20postrc again #12151

Merged

qinhanmin2014 approved these changes Sep 25, 2018

View reviewed changes

rth reviewed Sep 25, 2018

View reviewed changes

Update test_validation.py

3478036

FIX: changes other occurences

16290cd

rth approved these changes Sep 25, 2018

View reviewed changes

qinhanmin2014 approved these changes Sep 25, 2018

View reviewed changes

qinhanmin2014 merged commit 5431a1a into scikit-learn:master Sep 25, 2018

amueller pushed a commit that referenced this pull request Sep 25, 2018

BUG: check equality instead of identity in check_cv (#12155)

cbd65b4

Uh oh!

Conversation

glemaitre commented Sep 25, 2018

Uh oh!

glemaitre commented Sep 25, 2018

Uh oh!

glemaitre commented Sep 25, 2018

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 commented Sep 25, 2018

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

rth Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

rth Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

glemaitre Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Sep 25, 2018

Uh oh!

jorisvandenbossche commented Sep 25, 2018

Uh oh!

qinhanmin2014 commented Sep 25, 2018

Uh oh!

rth commented Sep 25, 2018

Uh oh!

glemaitre commented Sep 25, 2018

Uh oh!

rth commented Sep 25, 2018

Uh oh!

qinhanmin2014 commented Sep 25, 2018

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants