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] Add few more tests + Documentation for re-entrant cross-validation estimators #7823

Merged
merged 19 commits into from Jul 16, 2017

Conversation

Projects
None yet
3 participants
@raghavrv
Member

raghavrv commented Nov 4, 2016

TODO

  • Addres Joel's comment #7808 (comment)
  • Document that multiple split calls may not return identical train/test sets if random_state is not set.
  • There was a warning that suppressed a test. Using np.testing.assert_equal to test for equality of nested lists fixes that.
  • The word rank in the mock table view of cv_results_ is assumed as a link. Fixed in #7388
  • Address Andy's comments
  • Refactor @lesteve's #7946 out of this PR

@jnothman @amueller Pl. review :)

@raghavrv raghavrv changed the title from Model selection touchups to [MRG] Add few more tests + Documentation Nov 4, 2016

@raghavrv raghavrv added this to the 0.18.1 milestone Nov 4, 2016

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 5, 2016

Maybe it's easier just to document under random_state that the parameter is read in each call to split

@jnothman

Could you explain the issue with assert_equal a little more? perhaps it needs a comment.

@@ -816,7 +816,7 @@ class GridSearchCV(BaseSearchCV):
For instance the below given table
+------------+-----------+------------+-----------------+---+---------+
|param_kernel|param_gamma|param_degree|split0_test_score|...|rank_....|
|param_kernel|param_gamma|param_degree|split0_test_score|...| rank... |

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

Not better as rank_t...?

Note
----

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

I'd prefer no blank line here...

----
Multiple calls to the ``split`` method will not return identical
training or testing sets unless ``random_state`` is set to an integer

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

No such param

This comment has been minimized.

@raghavrv

raghavrv Nov 6, 2016

Member

TimeSeriesSplit has a random_state...

----
Multiple calls to the ``split`` method will not return identical
training or testing sets unless ``random_state`` is set to an integer

This comment has been minimized.

@jnothman

jnothman Nov 5, 2016

Member

only if shuffle=True

@raghavrv

This comment has been minimized.

Member

raghavrv commented Nov 6, 2016

@jnothman Have addressed your comments... Another look please?

np.testing.assert_equal(
np.array(list(kf_iter_wrapped.split(X, y))),
np.array(list(kf_randomized_iter_wrapped.split(X, y))))
except AssertionError:

This comment has been minimized.

@jnothman

jnothman Nov 7, 2016

Member

What's the point?

This comment has been minimized.

@raghavrv

raghavrv Nov 7, 2016

Member

Ah sorry you asked this before and I failed to give a response!

So the nested lists comparison raises a deprecation warning...

>>> assert_true(np.array(list(kfold.split(X, y))) != np.array(list(kfold.split(X, y))))
DeprecationWarning: elementwise != comparison failed; this will raise an error in the future.

np.testing.assert_equal on the other hand handles list of np.ndarrays gracefully...

This comment has been minimized.

@raghavrv

raghavrv Nov 7, 2016

Member

Argh wait I forgot the else clause. Sorry for that...

@@ -1175,12 +1175,30 @@ def test_grid_search_cv_splits_consistency():
cv=KFold(n_splits=n_splits))
gs2.fit(X, y)
# Give generator as a cv parameter

This comment has been minimized.

@jnothman

jnothman Nov 7, 2016

Member

To be sure, we've not got anything that ensures this will remain a generator. Either use a generator expression or test for its type.

This comment has been minimized.

@raghavrv

raghavrv Nov 7, 2016

Member

Done!

raghavrv added some commits Nov 7, 2016

list(kf_randomized_iter_wrapped.split(X, y)))
assert_true(np.any(np.array(list(kf_iter_wrapped.split(X, y))) !=
np.array(list(kf_randomized_iter_wrapped.split(X, y)))))
np.testing.assert_array_equal(

This comment has been minimized.

@jnothman

jnothman Nov 7, 2016

Member

still don't understand why you've resorted to np.testing.assert_array_equal

This comment has been minimized.

@raghavrv

raghavrv Nov 9, 2016

Member

np.testing.assert_array_equal handles nested lists unlike sklearn.testing.assert_array_equal...

This comment has been minimized.

@jnothman

jnothman Nov 10, 2016

Member

Well, I think this should at least be commented on somewhere in the file, if not imported into sklearn.utils.testing and given a different name, or the tests rewritten to do this comparison of nested lists explicitly.

@raghavrv

This comment has been minimized.

Member

raghavrv commented Nov 9, 2016

@jnothman @amueller Reviews?

list(kf_randomized_iter_wrapped.split(X, y)))
assert_true(np.any(np.array(list(kf_iter_wrapped.split(X, y))) !=
np.array(list(kf_randomized_iter_wrapped.split(X, y)))))
np.testing.assert_array_equal(

This comment has been minimized.

@jnothman

jnothman Nov 10, 2016

Member

Well, I think this should at least be commented on somewhere in the file, if not imported into sklearn.utils.testing and given a different name, or the tests rewritten to do this comparison of nested lists explicitly.

list(kf_randomized_iter_wrapped.split(X, y)))
assert_true(np.any(np.array(list(kf_iter_wrapped.split(X, y))) !=
np.array(list(kf_randomized_iter_wrapped.split(X, y)))))
# numpy's assert_array_equal properly compares nested lists

This comment has been minimized.

@raghavrv

raghavrv Nov 10, 2016

Member

@jnothman would this suffice? or would you prefer having this imported into sklearn.utils.testing rather?

This comment has been minimized.

@jnothman

jnothman Nov 18, 2016

Member

I suppose this comment is okay. Ideally I think we want all our asserts to come from one place and have clear naming for where they should be applied.

@raghavrv raghavrv modified the milestones: 0.19, 0.18.1 Nov 14, 2016

@amueller

We say that the splits are different with random_state=None, but that's not tested anywhere, is it?

Note
----
Multiple calls to the ``split`` method will not return identical
training or testing sets if ``random_state`` parameter exists and is

This comment has been minimized.

@amueller

amueller Nov 22, 2016

Member

Maybe "if splitting is randomized and the random_state parameter is not set to an integer"? I had to check where this was and what it means for a parameter to exist. Also, is that true? This class can not know how the subclasses treat the random_state.

So maybe rather "this might not be deterministic" which is not very explicit. Maybe describe this in the class docstring for each class and link there?

This comment has been minimized.

@amueller

amueller Nov 22, 2016

Member

Maybe it's my lack of coffee, but why does split use _iter_test_masks? Currently we create indices, transform them to booleans and then transform them back to indices. It is for making the negation easy?

This comment has been minimized.

@raghavrv

raghavrv Jun 29, 2017

Member

Sorry for coming to this very late. What do you suggest as the right thing to do?

"This *may not* be deterministic if `random_state`, if available, is not explicitly set while initializing the class"

Sounds okay to you?

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

How about "Randomized CV splitters may return different results for each call of split. This can be avoided (and identical results returned for each split) by setting random_state to an integer."

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

I think this belongs in the narrative docs too.

Note
----
Multiple calls to the ``split`` method will not return identical
training or testing sets unless ``random_state`` is set to an integer

This comment has been minimized.

@amueller

amueller Nov 22, 2016

Member

I'm not sure what "exists" means here. This class has a random_state parameter.

This comment has been minimized.

@amueller

amueller Nov 22, 2016

Member

Btw the if shuffle in the _make_test_fold method seems unnecessary and makes the code harder to follow imho.

This comment has been minimized.

@raghavrv

raghavrv Jun 29, 2017

Member

Same comment as above

This comment has been minimized.

@raghavrv

raghavrv Jun 29, 2017

Member

I am really unable to recollect why I did that :@ :@ :@ Arghh. I guess it was done so different split calls would produce same split? But I guess that was vetoed down (see: #7935).

I'll revert for now. I can reintroduce when someone complains.

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

Well, personally, I think having multiple split calls produce the same split is a better design, but not one that we currently implement.

def _pop_time_keys(cv_results):
for key in ('mean_fit_time', 'std_fit_time',
'mean_score_time', 'std_score_time'):
cv_results.pop(key)
return cv_results
# Check if generators as supported as cv and that the splits are consistent
np.testing.assert_equal(_pop_time_keys(gs3.cv_results_),

This comment has been minimized.

@amueller

amueller Nov 22, 2016

Member

wouldn't it be better to somehow test that the same samples have been used? We could have a model that stores the training data and a scorer that produces a hash of the training data in the model and the test data passed to score? Or is that too hacky for testing?

This comment has been minimized.

@raghavrv

raghavrv Jun 29, 2017

Member

Yeah that can be done. Maybe I'm being lazy but I feel this is sufficient? I can't see a case where this would pass if different samples were used. The score is quite sensitive to sample order no?

(Especially given that the dataset is make_classification and estimator is LinearSVC and not one of our toy estimators which would return 1 / 0 as the score)

@raghavrv raghavrv removed the Needs Review label Nov 29, 2016

@amueller amueller changed the title from [MRG] Add few more tests + Documentation to [MRG] Add few more tests + Documentation for re-entrant cross-validation estimators Jun 9, 2017

def _pop_time_keys(cv_results):
for key in ('mean_fit_time', 'std_fit_time',
'mean_score_time', 'std_score_time'):
cv_results.pop(key)
return cv_results
# Check if generators as supported as cv and that the splits are consistent

This comment has been minimized.

@raghavrv

raghavrv Jun 29, 2017

Member

are supported

@raghavrv

This comment has been minimized.

Member

raghavrv commented Jul 11, 2017

Gentle ping @jnothman @amueller :)

(Can we merge this now and address the rest if any later via an issue (maybe someone will pick it up during sprints)? My PR list is huge ;( )

Note
----
Multiple calls to the ``split`` method will not return identical
training or testing sets if ``random_state`` parameter exists and is

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

How about "Randomized CV splitters may return different results for each call of split. This can be avoided (and identical results returned for each split) by setting random_state to an integer."

Note
----
Multiple calls to the ``split`` method will not return identical
training or testing sets unless ``random_state`` is set to an integer

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

Well, personally, I think having multiple split calls produce the same split is a better design, but not one that we currently implement.

Note
----
Multiple calls to the ``split`` method will not return identical
training or testing sets if ``random_state`` parameter exists and is

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

I think this belongs in the narrative docs too.

@@ -1075,11 +1076,15 @@ def test_search_cv_results_rank_tie_breaking():
cv_results['mean_test_score'][2])

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

pytest: assert cv_results['mean_test_score'][1] != approx(cv_results['mean_test_score'][2])

:|

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

How about just: assert_false(np.allclose(cv_results['mean_test_score'][1], cv_results['mean_test_score'][2]))

shuffle=True, random_state=0).split(X, y),
GeneratorType))
# Give generator as a cv parameter

This comment has been minimized.

@jnothman

jnothman Jul 11, 2017

Member

If you really want to test that it's a generator, you should confirm (or ensure by using a generator expression) that it is indeed a generator. Otherwise the implementation in KFold may change and this test is no longer doing the right thing.

This comment has been minimized.

@jnothman

jnothman Jul 13, 2017

Member

you haven't addressed this.

This comment has been minimized.

@raghavrv

raghavrv Jul 15, 2017

Member

There is a test a few lines above (at L1431), which confirms if KFold indeed returns a GeneratorType

This comment has been minimized.

@jnothman

jnothman Jul 16, 2017

Member

Okay

But then the comment needs to appear before

@raghavrv

This comment has been minimized.

Member

raghavrv commented Jul 12, 2017

Thanks Joel, have addressed your comments

@@ -728,6 +728,10 @@ to shuffle the data indices before splitting them. Note that:
* To ensure results are repeatable (*on the same platform*), use a fixed value
for ``random_state``.
The randomized CV splitters may return different results for each call of

This comment has been minimized.

@jnothman

jnothman Jul 13, 2017

Member

This is described in less clear terms in the preceding bullet point. Please merge them.

shuffle=True, random_state=0).split(X, y),
GeneratorType))
# Give generator as a cv parameter

This comment has been minimized.

@jnothman

jnothman Jul 13, 2017

Member

you haven't addressed this.

Note
----
Randomized CV splitters may return different results for each call of
split. This can be avoided (and identical results returned for each

This comment has been minimized.

@amueller

amueller Jul 15, 2017

Member

This seems a bit redundant. Maybe "You can make the results identical by setting random_state to an integer"?

@amueller

This comment has been minimized.

Member

amueller commented Jul 15, 2017

this looks good but I haven't double checked if it addressed all of @jnothman's comments from the original PR.

@raghavrv

This comment has been minimized.

Member

raghavrv commented Jul 16, 2017

Done. If you guys are happy, this can be merged now. Unless travis decides to give me a headache.

@jnothman jnothman merged commit 75d6005 into scikit-learn:master Jul 16, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.15%)
Details
codecov/project 96.18% (+0.03%) compared to b6f8865
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnothman

This comment has been minimized.

Member

jnothman commented Jul 16, 2017

Thanks

@raghavrv raghavrv deleted the raghavrv:model_selection_touchups branch Jul 17, 2017

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Aug 6, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG] Add few more tests + Documentation for re-entrant cross-validat…
…ion estimators (scikit-learn#7823)

* DOC Add NOTE that unless random_state is set, split will not be identical

* TST use np.testing.assert_equal for nested lists/arrays

* TST Make sure cv param can be a generator

* DOC rank_ becomes a link when rendered

* Use test_...

* Remove blank line; Add if shuffle is True

* Fix tests

* Explicitly test for GeneratorType

* TST Add the else clause

* TST Add comment on usage of np.testing.assert_array_equal

* TYPO

* MNT Remove if ;

* Address Joel's comments

* merge the identical points in doc

* DOC address Andy's comments

* Move comment to before the check for generator type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment