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

raghavrv · 2016-11-25T16:47:57Z

A better patch over my previous #7660 for ensuring consistency of splits (and in parameters) without the additional memory required to store the cv indices (as was done in #7660). This should hopefully address the concerns raised there reg. the memory consumption.

@jnothman @GaelVaroquaux @amueller All the tests seem to pass cleanly.

raghavrv · 2016-11-25T16:49:50Z

sklearn/model_selection/_search.py

-        if self.verbose > 0 and isinstance(parameter_iterable, Sized):
-            n_candidates = len(parameter_iterable)
+        # Regenerate parameter iterable for each fit
+        candidate_params = list(self._param_iterable)


This stores all the dicts as a list but this should not cause any (additional) memory (compared to 0.18) as it is anyway being stored a couple of lines later. Infact since we do not return the parameters and store them at out, it should essentially reduce the memory consumption..

raghavrv · 2016-11-26T18:29:30Z

Also I couldn't find a (non-hacky) way to safely not use lists in learning_curve which uses the cv splits multiple times.

One think I think would be appropriate there (and elsewhere) would be to try iterate the cv twice to check if it is reentrant and raise an error asking them to pass a reentrant one or materialize outside and pass it as a list to us making things more explicit. Or if you feel it is fine to have it as such for learning_curve alone, then we are good to go here I think...

This and #7935 should fix most/all of the concerns raised previously while being backward compatible with 0.17...

amueller · 2016-11-28T22:53:43Z

sklearn/model_selection/_search.py

@@ -532,25 +532,51 @@ def inverse_transform(self, Xt):
        self._check_is_fitted('inverse_transform')
        return self.best_estimator_.transform(Xt)

-    def _fit(self, X, y, groups, parameter_iterable):
-        """Actual fitting,  performing the search over parameters."""
+    @property


Why not use polymorphism? This looks like reimplementing polymorphism based on attribute names...

amueller · 2016-11-28T22:54:30Z

sklearn/model_selection/_validation.py

    cv_iter = list(cv.split(X, y, groups))
+


why are we still materializing here?

I don't think we can do away with materializing for learning_curve. (Also it was like that before pre-0.18)

jnothman · 2016-11-29T07:57:39Z

sklearn/model_selection/_search.py

@@ -532,25 +532,51 @@ def inverse_transform(self, Xt):
        self._check_is_fitted('inverse_transform')
        return self.best_estimator_.transform(Xt)

-    def _fit(self, X, y, groups, parameter_iterable):
-        """Actual fitting,  performing the search over parameters."""
+    @property


I don't see how this helps anything and perhaps the previous version was easier to read...?

raghavrv · 2016-12-02T17:15:51Z

@jnothman It combines the 2 fit functions and saves us some lines. Could you check if after the recent changes, you still feel having separate fit functions is a better option?

raghavrv · 2016-12-02T17:16:21Z

@amueller Have used proper polymorphism... One more pass?

jnothman

Tests aren't running due to syntax error, otherwise the code looks functionally fine.

I'm still not sure _param_iterable is the right design. It is very anti-functional to store something on the object that then somewhat-secretly gets accessed by fit.

jnothman · 2016-12-05T02:17:23Z

sklearn/model_selection/_search.py

+    @property
+    def _param_iterable(self):
+        """To generate parameter iterables for multiple iterations"""
+            return ParameterGrid(self.param_grid)


IndentationError

raghavrv · 2016-12-05T15:01:02Z

@jnothman I've used an explicit getter function instead of property... Let me know if this is better. If not we could revert back to the previous setup of separate fit calls per class...

jnothman · 2016-12-05T20:42:26Z

If we want to encourage users to make their own BaseSearchCV extensions, I suppose the latest version is best...?

…

On 6 December 2016 at 02:01, Raghav RV ***@***.***> wrote: @jnothman <https://github.com/jnothman> I've used an explicit getter function instead of property... Let me know if this is better. If not we could revert back to the previous setup of separate fit calls per class... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7941 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6yDXCjMq0QxYc8FWtk5WCqXOPPPAks5rFCcvgaJpZM4K8mK2> .

raghavrv · 2016-12-05T21:21:11Z

the latest version

By latest version, you mean the PR in its current state? If yes, I need to make an abstract @get_parameter_iterable in the BaseSearchCV...

amueller

LGTM

amueller · 2016-12-05T22:17:18Z

sklearn/model_selection/_search.py

-          for parameters in parameter_iterable
-          for train, test in cv_iter)
+          for train, test in cv.split(X, y, groups)
+          for parameters in candidate_params)


return_parameters=False is unrelated, right? but it seems fine.

amueller · 2016-12-05T22:21:20Z

sklearn/model_selection/_validation.py

@@ -128,7 +128,6 @@ 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 = list(cv.split(X, y, groups))


why did we put this here at all?

Paranoia on my part I guess when I learnt model_selection doesn't handle one time cv-splitters and rushed to push in a fix... :/

raghavrv · 2016-12-05T22:45:14Z

Have added your +1. Could you also github approve?

raghavrv · 2016-12-05T22:59:36Z

Thanks :)

jnothman · 2016-12-05T23:25:17Z

Yes, make an abstract `get_parameter_iterable`. But actually, it should be `get_parameter_iterator`, no? Whether we then make BaseSearchCV public (!), I'm not sure...

…

On 6 December 2016 at 09:59, Raghav RV ***@***.***> wrote: Thanks :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7941 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz606bVzck7qhYWwQaYelveSL-CYNSks5rFJdagaJpZM4K8mK2> .

amueller · 2016-12-06T00:03:26Z

I would not make it public and not add abstract method. If someone wants to implement their own, let them. But I don't think there is a lot of worth in adding this to the public interface.

raghavrv · 2016-12-06T11:56:53Z

Okay with that @jnothman are you +1 for merge then?

jnothman · 2016-12-06T23:02:50Z

Without further resolve, we need to make it _get_param_iterator (private)

raghavrv · 2016-12-06T23:06:19Z

Done... ;)

jnothman · 2016-12-07T10:13:07Z

LGTM

yogeveran · 2016-12-12T14:57:25Z

cross_val_predict does not work with TimeSeriesSplit.
It raises there error here because some records are not predicted on.
if not _check_is_permutation(test_indices, _num_samples(X)): raise ValueError('cross_val_predict only works for partitions')

raghavrv · 2016-12-12T15:34:11Z

@yogeveran Could you raise a new issue at https://github.com/scikit-learn/scikit-learn/issues. Make sure to include a sample snippet which highlights your issue...

ogrisel · 2017-01-07T12:17:40Z

I think we should backport this fix to the 0.18.X branch if we plan to release 0.18.2.

jnothman · 2017-01-07T12:19:37Z

Perhaps although the only time we've seen the issue raised this was the wrong diagnosis.

…

On 7 January 2017 at 23:17, Olivier Grisel ***@***.***> wrote: I think we should backport this fix to the 0.18.X branch if we plan to release 0.18.2. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#7941 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_ivcw5XASDg9fat5A_w11eoUvxHks5rP4JlgaJpZM4K8mK2> .

…earn#7941) * ENH Parallelize by candidates first then by splits. * ENH do not materialize a cv iterator to avoid memory blow ups.

raghavrv added 2 commits November 25, 2016 17:30

ENH Parallelize by candidates first then by splits.

b70ffb6

ENH do not materialize a cv iterator to avoid memory blow ups.

740d3d2

raghavrv added Blocker Enhancement labels Nov 25, 2016

raghavrv added this to the 0.18.2 milestone Nov 25, 2016

raghavrv commented Nov 25, 2016

View reviewed changes

raghavrv mentioned this pull request Nov 28, 2016

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

Closed

amueller reviewed Nov 28, 2016

View reviewed changes

jnothman reviewed Nov 29, 2016

View reviewed changes

raghavrv removed this from the 0.18.2 milestone Nov 30, 2016

amueller added this to the 0.19 milestone Nov 30, 2016

Use real polymorphism

de43c92

jnothman reviewed Dec 5, 2016

View reviewed changes

Use getter instead of property

932891c

amueller reviewed Dec 5, 2016

View reviewed changes

raghavrv changed the title ~~[MRG] ENH Ensure consistency in splits and in parameters (without causing memory blowup because of materializing the iterator)~~ [MRG + 1] ENH Ensure consistency in splits and in parameters (without causing memory blowup because of materializing the iterator) Dec 5, 2016

amueller approved these changes Dec 5, 2016

View reviewed changes

raghavrv added the Waiting for Reviewer label Dec 5, 2016

param_iterable --> param_iterator

2bd348f

Make get_param_iterator pvt.

1efe648

jnothman merged commit 3dcb873 into scikit-learn:master Dec 7, 2016

raghavrv deleted the consistency_in_splits branch December 12, 2016 15:34

ogrisel modified the milestones: 0.18.2, 0.19 Jan 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raghavrv commented Nov 25, 2016

raghavrv Nov 25, 2016

raghavrv commented Nov 26, 2016

amueller Nov 28, 2016

amueller Nov 28, 2016

raghavrv Dec 2, 2016

jnothman Nov 29, 2016

raghavrv commented Dec 2, 2016

raghavrv commented Dec 2, 2016

jnothman left a comment

jnothman Dec 5, 2016

raghavrv commented Dec 5, 2016

jnothman commented Dec 5, 2016 via email

raghavrv commented Dec 5, 2016

amueller left a comment

amueller Dec 5, 2016

amueller Dec 5, 2016

raghavrv Dec 5, 2016 •

edited

Loading

raghavrv commented Dec 5, 2016

raghavrv commented Dec 5, 2016

jnothman commented Dec 5, 2016 via email

amueller commented Dec 6, 2016

raghavrv commented Dec 6, 2016

jnothman commented Dec 6, 2016

raghavrv commented Dec 6, 2016

jnothman commented Dec 7, 2016

yogeveran commented Dec 12, 2016 •

edited

Loading

raghavrv commented Dec 12, 2016

ogrisel commented Jan 7, 2017

jnothman commented Jan 7, 2017 via email

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

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

Conversation

raghavrv commented Nov 25, 2016

Choose a reason for hiding this comment

raghavrv commented Nov 26, 2016

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

raghavrv commented Dec 2, 2016

raghavrv commented Dec 2, 2016

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv commented Dec 5, 2016

jnothman commented Dec 5, 2016 via email

raghavrv commented Dec 5, 2016

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

raghavrv commented Dec 5, 2016

raghavrv commented Dec 5, 2016

jnothman commented Dec 5, 2016 via email

amueller commented Dec 6, 2016

raghavrv commented Dec 6, 2016

jnothman commented Dec 6, 2016

raghavrv commented Dec 6, 2016

jnothman commented Dec 7, 2016

yogeveran commented Dec 12, 2016 • edited Loading

raghavrv commented Dec 12, 2016

ogrisel commented Jan 7, 2017

jnothman commented Jan 7, 2017 via email

raghavrv Dec 5, 2016 •

edited

Loading

yogeveran commented Dec 12, 2016 •

edited

Loading