[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split calls #7660

raghavrv · 2016-10-13T02:34:03Z

To make consistent splits for all parameter settings when the cv iterators are non re-entrant (thanks to Joel for the word!) and non deterministic for successive split calls, we call split once and store it as a list that will be iterated multiple times.
Similarly in _CVIterableWrapper, we store the list rather than the cv object and iterate on it... (NOTE that this would make previously (intentionally) non deterministic cv iterables, deterministic with consistent output for successive split calls. Do we want that to happen?)

raghavrv · 2016-10-13T02:34:31Z

raghavrv · 2016-10-13T02:36:25Z

Also what kind of tests should I add here? I can make a mock splitter which will iterate only once and pass it as cv to grid search. That will fail in master but will pass in this branch. Any other tests?

jnothman · 2016-10-13T03:14:14Z

passing in a generator expression for cv would be a sufficient non-regression test.

Without tests, surely this should be WIP?

raghavrv · 2016-10-13T03:25:40Z

Without tests, surely this should be WIP?

Indeed. Sorry for marking it MRG

raghavrv · 2016-10-13T05:19:42Z

Done! Reviews please ;)

amueller · 2016-10-13T22:09:34Z

sklearn/model_selection/_validation.py

@@ -129,6 +128,7 @@ def cross_val_score(estimator, X, y=None, groups=None, scoring=None, cv=None,
    X, y, groups = indexable(X, y, groups)

    cv = check_cv(cv, y, classifier=is_classifier(estimator))
+    cv_iter = cv.split(X, y, groups)


why this change?

Just as a sign of best practice denoting that we enlist the indices and store them as soon as we check the cv param...

I'm not sure I get the point. cv_iter is a generator, right? This adds a line and doesn't make the code easier to follow.

argh sorry I somehow kept thinking I did list(cv.split(...)...

amueller · 2016-10-13T22:10:10Z

sklearn/model_selection/_validation.py

@@ -384,6 +384,7 @@ def cross_val_predict(estimator, X, y=None, groups=None, cv=None, n_jobs=1,
    X, y, groups = indexable(X, y, groups)

    cv = check_cv(cv, y, classifier=is_classifier(estimator))
+    cv_iter = cv.split(X, y, groups)


(Same comment as above)

Should I leave it as before?

amueller · 2016-10-13T22:10:40Z

sklearn/model_selection/tests/test_search.py

+                'mean_fit_time', 'std_fit_time'):
+        gs.cv_results_.pop(key)
+        gs2.cv_results_.pop(key)
+    np.testing.assert_equal(gs.cv_results_, gs2.cv_results_)


why not scikit learn testing?

The numpy's testing.assert_equal is to check equivalence of nested dicts. We don't have an equivalent AFAIK...

amueller · 2016-10-13T22:11:44Z

you're not testing the consistency of folds in GridSearchCV or learning_curve or validation_curve.

amueller · 2016-10-14T20:07:20Z

sklearn/model_selection/tests/test_search.py

+        def __init__(self, n_samples=100):
+            self.indices = KFold(n_splits=5).split(np.ones(n_samples))
+
+        def split(self, X=None, y=None, groups=None):


maybe add a comment that says that this split can only be called once.

amueller · 2016-10-14T20:09:12Z

sklearn/model_selection/tests/test_search.py

+                            _pop_time_keys(gs2.cv_results_))
+
+    # Check consistency of folds across the parameters
+    gs = GridSearchCV(LinearSVC(random_state=0),


Did this test fail before? Maybe also add a one-line comment explaining what's happening.

Yes it fails on master...

raghavrv · 2016-10-16T17:27:46Z

@amueller Have fixed your comments. I'm not able to find a way to test the consistency of cv folds in learning_curve, the learning curve seems to filter duplicate values for train sizes. Otherwise I could have given two similar values and test their scores as a proof of consistency in splits...

Other than that, I have addressed all your comments, added explanatory comments in tests + all tests pass. One more review please?

cc: @jnothman too if you find time... :)

raghavrv · 2016-10-16T17:42:03Z

sklearn/model_selection/_split.py

@@ -1467,7 +1467,7 @@ def get_n_splits(self, X=None, y=None, groups=None):
 class _CVIterableWrapper(BaseCrossValidator):
    """Wrapper class for old style cv objects and iterables."""
    def __init__(self, cv):
-        self.cv = cv
+        self.cv = list(cv)


@jnothman @amueller

Do we want this change? This would mean check_cv(old_type_one_time_iterator) --> new type multi entrant iterator?
I am not able to dig through the huge pile of conversations, but I recall us choosing this for the reason that if the iterable that is wrapped is non re-entrant the wrapped splitter should also be non re-entrant...

For instance

import numpy as np from sklearn.model_selection import KFold from sklearn.model_selection import check_cv from sklearn.datasets import make_classification from sklearn.utils.testing import assert_array_equal X, y = make_classification(random_state=0) # At master - This test will pass ------------------------------------------ kf_iter = KFold(n_splits=5).split(X, y) kf_iter_wrapped = check_cv(kf_iter) # First split call will work list(kf_iter_wrapped.split(X, y)) # But not subsequent assert_array_equal(list(kf_iter_wrapped.split(X, y)), []) # At this PR - This test will pass ------------------------------------------ kf_iter = KFold(n_splits=5).split(X, y) kf_iter_wrapped = check_cv(kf_iter) # Since the wrapped iterable is enlisted and stored, # split can be called any number of times to produce # consistent results assert_array_equal( list(kf_iter_wrapped.split(X, y)), list(kf_iter_wrapped.split(X, y)))

I think this change is fine. This is only used internally, right? All sklearn functions worked with a single-run iterator before, and now they will do that again after this PR. So there are no behavior changes, right?
If someone used check_cv they will get something different now, which is not entirely great, but this is a bug-fix on 0.18 so we have to change behavior. We never promised our wrapper would be one-iteration if the original cv was.

Ok thanks for the comment...

(Just to note that I've added the 2nd test just to ensure this change has effect and to fail if reverted later...)

I suppose infinite CV strategies are unlikely. Expensive ones are a bit more likely, but I think this approach is okay.

Do we revert this change too? @jnothman @amueller?

amueller · 2016-10-17T17:44:46Z

sklearn/model_selection/tests/test_search.py

@@ -61,6 +61,22 @@
 from sklearn.linear_model import SGDClassifier


+class CustomSplitter():


If this is used in two tests, it should probably be in utils.testing.

amueller · 2016-10-17T17:45:03Z

sklearn/model_selection/tests/test_validation.py

@@ -61,6 +61,7 @@
 from sklearn.datasets import make_multilabel_classification

 from sklearn.model_selection.tests.test_split import MockClassifier


I don't think tests should import from other tests.

There are quite a few MockClassifier's that are specific to the tests and the module. I'm not sure there is much benefit in grouping them into utils.testing...

Well if they are specific to a test, then I think they should stay there. If they are not, then maybe they should be shared? I'm not sure.

Other option would be to have a separate private file (in the tests directory) for these kind of classifiers/mock splitters and import from that?

Well if they are specific to a test, then I think they should stay there.

They are specific to 2 sets of test in 2 different test files, but not anywhere else except the model_selection tests, hence my disinclination to move them to utils.testing :)

Maybe you want them moved to sklearn.model_selection.tests._test_setup?

amueller · 2016-10-17T17:45:48Z

sklearn/model_selection/tests/test_validation.py

+            cv=CustomSplitter(n_splits=n_splits, n_samples=30),
+            train_sizes=np.linspace(0.1, 1.0, 10))
+    if len(w) > 0:
+        raise RuntimeError("Unexpected warning: %r" % w[0].message)


AssertionError?

Or just do an assert instead of an if?

apart from this LGTM

jnothman · 2016-10-19T14:13:50Z

Elsewhere we have ./sklearn/cluster/tests/common.py

On 20 October 2016 at 01:10, Raghav RV notifications@github.com wrote:

@raghavrv commented on this pull request.

In sklearn/model_selection/tests/test_validation.py
#7660:

@@ -61,6 +61,7 @@
from sklearn.datasets import make_multilabel_classification

from sklearn.model_selection.tests.test_split import MockClassifier

Well if they are specific to a test, then I think they should stay there.

They are specific to 2 sets of test in 2 different test files, but not
anywhere else except the model_selection tests, hence my disinclination
to move them to utils.testing :)

Maybe you want them moved to sklearn.model_selection.tests._test_setup?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7660, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAEz6zAtCX1DnzgNqZ-zMHJ7hicL9514ks5q1iTvgaJpZM4KVa_v
.

raghavrv · 2016-10-19T14:16:09Z

Elsewhere we have ./sklearn/cluster/tests/common.py

Thanks for checking. I'll move them to model_selection/tests/common.py...

jnothman · 2016-11-02T10:33:20Z

split(..) evaluates to a generator, not a generator function.

We could call split multiple times in gscv, but at the risk that multiple calls return distinct splits, as they currently do if random_state is a non-int in any that support shuffle=True.

jnothman · 2016-11-02T10:34:48Z

i wouldn't usually support materialising an iterator as a list unnecessarily, but I think this is the solution of least surprise to the user, especially as CV splits aren't usually expensive to compute or store relative to the rest of the problem, and their number is necessarily finite and predetermined, at least in gscv/rscv.

… calls (scikit-learn#7660)

amueller · 2016-11-14T13:25:50Z

this needs a whatsnew.

GaelVaroquaux · 2016-11-21T22:46:59Z

I think that the solutions used in this PR are not correct: it is important to rely on iterators and not lists. Lists are leading to heavy data duplication #7919

raghavrv · 2016-11-21T23:05:35Z

Ahh okay should we resort to itertools.tee then?

jnothman · 2016-11-21T23:29:39Z

itertools.tee effectively stores things. We may just have to prohibit
non-reentrant iterators??

On 22 November 2016 at 10:05, Raghav RV notifications@github.com wrote:

Ahh okay should we resort to itertools.tee
https://docs.python.org/2/library/itertools.html#itertools.tee then?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7660 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz690iKqNOWD-N12f1bWVIQNKtdVXfks5rAiPAgaJpZM4KVa_v
.

raghavrv · 2016-11-21T23:32:50Z

I'm +1 for that. That would simplify a few things + discourage hacked up cvs that do not conform to our splitter API (Though I am not sure if reentrancy is kind of implied by our API)...

jnothman · 2016-11-22T00:30:03Z

I think this still comes back to things like randomization between calls to split(). I'll try think about this more later.

:(

amueller · 2016-11-23T22:08:26Z

@GaelVaroquaux how do you ensure that an generator returns identical iterators when called twice without storing it?
So we could give up on guaranteeing consistency across runs using arbitrary cross-validation iterators, and make sure ours are generating consistent sequences?

What do you think should be the semantics of passing KFold(shuffle=True) to GridSearchCV?
Our previous behavior (pre 0.18 and post 0.18.1) is that all parameters have the same folds.
In fact, iterating of KFold(shuffle=True) any number of times used to create exactly the same folds. Since 0.18, each iteration over KFold(shuffle=True) returns different folds.
And that is something we were not really careful enough about :(

I would argue that having it return always the same sequence is somewhat counter-intuitive. What do you think?

GaelVaroquaux · 2016-11-23T22:23:28Z

@GaelVaroquaux how do you ensure that an generator returns identical iterators
when called twice without storing it?

Well, a generator strictly speak not. But we could ensure that the split
method of the CV object returns the same thing.

I would argue that having it return always the same sequence is somewhat
counter-intuitive. What do you think?

In terms of intuition, maybe if you didn't specific the random_state, I
would agree with you.

However, that's not a nice semantic, because it will require the user to
store the list of indices generated in many cases. For instance when
running multiple estimators and wanting to compare the results. It will
be easy to get it wrong. If you look at the bug that was fixed by
@raghavrv, I believe that the code would be wrong without the list if the
iteration was not reproducible.
https://github.com/scikit-learn/scikit-learn/pull/7660/files#diff-676ff6854a280680ed7d91103ef4b0b5R553

The list expands the generator, and hence avoids any irreproducibility
and side effects. But in a sense it defeats the purpose of having
generators. If we consider the extreme case of really large n small p
(say p = 30, and n in the millions), doing 100 iterations of
shuffle-split would create more indices that are stored in the memory
than actual data.

amueller · 2016-11-23T22:27:11Z

The semantics of always producing the same result are somewhat nicer than what's there now, and yes, using lists defeats the purpose of iterators.
We can go back to checking the random_state on __init__ if we agree that "deterministic randomness" is fine. We should be very explicit about that, though.

GaelVaroquaux · 2016-11-23T22:28:47Z

We can go back to checking the random_state on init if we agree that
"deterministic randomness" is fine. We should be very explicit about that,
though.

With insight, I think that this is better. The bug and corresponding fix
point it out clearly, IMHO.

raghavrv · 2016-11-23T22:31:09Z

Since 0.18, each iteration over KFold(shuffle=True) returns different folds.

If that is the only concern here we could change the order of iteration here. It would paralelize over parameters rather than splits. The problem with that approach is it could be slower when one parameter setting happens to train faster than another, unless joblib is smart to figure that out and let the next job take over that core. (For instance say I iterate over n_estimators=[1, 10, 100, 2000] of random forest, then for a 4 core machine the first core could be idle unless joblib has some nice heuristics that I am unaware of..)

jnothman · 2016-11-24T01:25:18Z

Joblib has no problem with pooling and taking the next job off the queue afaik, so that's not an issue.

raghavrv · 2016-11-24T14:48:40Z

Shall I proceed with the PR. Are you okay with that approach @amueller @GaelVaroquaux?

raghavrv · 2016-11-24T15:18:35Z

And for the problem of shuffle in spliltters what do you think of setting a random int as random_state if random_state is None.

And then reusing this guy to regenerate rng at split?

# At init
if random_state is None:
    random_state = check_random_state(random_state).randint(0, 100000)

# At split
rng = check_random_state(random_state)

Would take care of determinism for successive splits and will also ensure the splits are random for multiple initialization but consistent across splits after initialization.

raghavrv · 2016-11-24T15:23:24Z

If, after both my changes, all tests pass we are good to go I think...

raghavrv · 2016-11-24T17:01:32Z

we could change the order of iteration here.

Sweeeet now we have to worry about one time parameter samplers / grid iterators ;@ I think copying / changing parameter grid / sampler will break user code more than explicitly not supporting non-reentrant cvs. A lot of users hack the ParameterGrid in a variety of ways that may not be compatible with copying et all...

raghavrv · 2016-11-24T17:04:29Z

I think we can have #7935, revert all the list(iter) changes brought about by this PR and add a note at cv doc stating that multiple iterations are done and hence we expect the cv to not be a non-reentrant one. If they want they can materialize the iterator externally... WDYT?

raghavrv · 2016-11-25T16:16:52Z

Sweeeet now we have to worry about one time parameter samplers / grid iterators

Wait since all the parameter dicts are anyway generated and stored, it wouldn't actually hurt to materialize that as a list and store them. If my intuition here is correct, doing this will not consume any additional memory...

jnothman · 2016-11-28T01:38:48Z

Yes, we store parameter lists anyway.

…

On 26 November 2016 at 03:16, Raghav RV ***@***.***> wrote: Sweeeet now we have to worry about one time parameter samplers / grid iterators Wait since all the parameter dicts are anyway generated and stored, it wouldn't actually hurt to materialize that as a list and store them. If my intuition here is correct, doing this will not consume any additional memory... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7660 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_P41GgVCASELpUSfkZ_kZQhP3KMks5rBwn1gaJpZM4KVa_v> .

… calls (scikit-learn#7660)

raghavrv added the Waiting for Reviewer label Oct 13, 2016

raghavrv added this to the 0.18.1 milestone Oct 13, 2016

raghavrv mentioned this pull request Oct 13, 2016

[architectural suggestion] Move random number generator to initializer in model_selection._split #6726

Closed

raghavrv changed the title ~~[MRG] FIX Be robust to non re-entrant/ non deterministic cv.split calls~~ [WIP] FIX Be robust to non re-entrant/ non deterministic cv.split calls Oct 13, 2016

raghavrv removed the Waiting for Reviewer label Oct 13, 2016

raghavrv changed the title ~~[WIP] FIX Be robust to non re-entrant/ non deterministic cv.split calls~~ [MRG] FIX Be robust to non re-entrant/ non deterministic cv.split calls Oct 13, 2016

raghavrv added the Waiting for Reviewer label Oct 13, 2016

amueller reviewed Oct 13, 2016

View reviewed changes

raghavrv force-pushed the use_split_only_once branch from 1898288 to d8d42a8 Compare October 14, 2016 15:37

amueller reviewed Oct 14, 2016

View reviewed changes

raghavrv commented Oct 16, 2016

View reviewed changes

amueller added the Blocker label Oct 17, 2016

amueller reviewed Oct 17, 2016

View reviewed changes

raghavrv force-pushed the use_split_only_once branch from cf986ed to d84b517 Compare October 19, 2016 19:13

raghavrv added 2 commits October 19, 2016 21:20

FIX Be robust to non re-entrant/ non deterministic cv.split calls

7067018

TST Ensure custom one time iterators are accepted as cv

eab1fe7

amueller pushed a commit to amueller/scikit-learn that referenced this pull request Nov 9, 2016

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split…

7e1bef5

… calls (scikit-learn#7660)

raghavrv mentioned this pull request Nov 14, 2016

[MRG + 1] DOC Add whatsnew for the 3 model_selection bugfixes #7868

Merged

GaelVaroquaux mentioned this pull request Nov 21, 2016

Avoid duplicating data in cross_val_score #7919

Closed

raghavrv mentioned this pull request Nov 24, 2016

[MRG + 1] Set a random random_state at init to ensure deterministic randomness when random_state is None #7935

Closed

raghavrv mentioned this pull request Nov 25, 2016

[MRG + 1] ENH Ensure consistency in splits and in parameters (without causing memory blowup because of materializing the iterator) #7941

Merged

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split…

1564f11

… calls (scikit-learn#7660)

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split…

a4ac609

… calls (scikit-learn#7660)

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

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split…

d836fc1

… calls (scikit-learn#7660)

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

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split…

8d731ee

… calls (scikit-learn#7660)

		@@ -61,6 +61,22 @@
		from sklearn.linear_model import SGDClassifier


		class CustomSplitter():

		@@ -61,6 +61,7 @@
		from sklearn.datasets import make_multilabel_classification

		from sklearn.model_selection.tests.test_split import MockClassifier

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split calls #7660

[MRG + 2] FIX Be robust to non re-entrant/ non deterministic cv.split calls #7660

Conversation

raghavrv commented Oct 13, 2016

raghavrv commented Oct 13, 2016

raghavrv commented Oct 13, 2016

jnothman commented Oct 13, 2016

raghavrv commented Oct 13, 2016

raghavrv commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv commented Oct 16, 2016

raghavrv Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Oct 19, 2016

@raghavrv commented on this pull request.

raghavrv commented Oct 19, 2016

jnothman commented Nov 2, 2016

jnothman commented Nov 2, 2016

amueller commented Nov 14, 2016

GaelVaroquaux commented Nov 21, 2016

raghavrv commented Nov 21, 2016

jnothman commented Nov 21, 2016

raghavrv commented Nov 21, 2016

jnothman commented Nov 22, 2016

amueller commented Nov 23, 2016

GaelVaroquaux commented Nov 23, 2016

amueller commented Nov 23, 2016

GaelVaroquaux commented Nov 23, 2016

raghavrv commented Nov 23, 2016 • edited Loading

jnothman commented Nov 24, 2016

raghavrv commented Nov 24, 2016

raghavrv commented Nov 24, 2016 • edited Loading

raghavrv commented Nov 24, 2016

raghavrv commented Nov 24, 2016 • edited Loading

raghavrv commented Nov 24, 2016 • edited Loading

raghavrv commented Nov 25, 2016

jnothman commented Nov 28, 2016 via email

amueller Oct 14, 2016 •

edited

Loading

raghavrv Oct 14, 2016 •

edited

Loading

raghavrv Oct 16, 2016 •

edited

Loading

raghavrv commented Nov 23, 2016 •

edited

Loading

raghavrv commented Nov 24, 2016 •

edited

Loading

raghavrv commented Nov 24, 2016 •

edited

Loading

raghavrv commented Nov 24, 2016 •

edited

Loading