[MRG+1] add option to cross_validate to return estimators fitted on each split #9686

bellet · 2017-09-04T16:41:24Z

Reference Issues

Fixes #6827 (alternative to #9496)

What does this implement/fix? Explain your changes.

This PR adds an option to cross_validate so that it returns a list of the estimators fitted on each split. This list is an additional entry to the returned dictionary.

Any other comments?

I simply wrote some basic code implementing the change. If core devs reach some consensus in favor of this solution, I will go ahead and improve the code, add tests and update the doc.

…split

jnothman · 2017-09-04T22:58:41Z

it's always a good idea to include a test to show that the implementation does what you think it does. On 5 Sep 2017 2:41 am, "Aurélien Bellet" <notifications@github.com> wrote: Reference Issues Fixes #6827 <#6827> (alternative to #9496 <#9496>) What does this implement/fix? Explain your changes. This PR adds an option to cross_validate so that it returns a list of the estimators fitted on each split. This list is an additional entry to the returned dictionary. Any other comments? I simply wrote some basic code implementing the change. If there is some consensus in favor of this solution, I will go ahead and improve the code, add tests and update the doc. ------------------------------ You can view, comment on, or merge this pull request online at: #9686 Commit Summary - add option to cross_validate to return the estimators fitted on each split File Changes - *M* sklearn/model_selection/_validation.py <https://github.com/scikit-learn/scikit-learn/pull/9686/files#diff-0> (35) Patch Links: - https://github.com/scikit-learn/scikit-learn/pull/9686.patch - https://github.com/scikit-learn/scikit-learn/pull/9686.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9686>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz64-xBheNjbtNCrD_k6xx27W8IShQks5sfCg1gaJpZM4PMHJx> .

jnothman

I'm fine with this kind of change... But I'm not sure if we will find consensus, and @GaelVaroquaux has generally been against giving the user the kitchen sink (to use a strange English expression for "everything and more"). It also has the potential to snowball and become a feature request in grid search. Even if it does not take excessive memory here, it may there. And someone will complain that it falls to return some un-picklable they have created.

Thanks for making this concrete, but let's consider an alternative: Is there a way to instead allow the user to specify a filename format and dump the estimator to disk?

jnothman · 2017-09-04T22:59:35Z

sklearn/model_selection/_validation.py

-        train_scores, test_scores, fit_times, score_times = zip(*scores)
+        if return_estimator:
+            (train_scores, test_scores, fit_times, score_times,
+             fitted_est) = zip(*scores)


For good or bad, all the other return values here are named in plural. This should be consistent unless there's a very good reason not to be.

jnothman · 2017-09-04T23:03:00Z

sklearn/model_selection/_validation.py

        for train, test in cv.split(X, y, groups))

    if return_train_score:
-        train_scores, test_scores, fit_times, score_times = zip(*scores)
+        if return_estimator:


There must be a neater way to write this without nested ifs. But alas I'm not sure what it is.

bellet · 2017-09-05T08:34:21Z

Or a callback taking a fitted estimator as input?

jnothman · 2017-09-05T08:36:51Z

I'm afraid the callback approach is plagued by the potential for other parameters: passing the training data, the test data, the fold no, etc. And then we become a framework.

bellet · 2017-09-05T09:40:18Z

Fair enough. I can implement dumping the estimator to disk option if there is some consensus that this is the way to go?

GaelVaroquaux · 2017-09-05T09:42:44Z

and @GaelVaroquaux has generally been against giving the user the kitchen sink (to use a strange English expression for "everything and more"). It also has the potential to snowball and become a feature request in grid search.

I am OK with this, as long as we give limitations in the docstring (ie: beware that all models are not picklable) and as long as really don't get this in GridSearch. My logic is that if it's a contained kitchen sink, it's OK. I am worried about the overflowing kitchen sink :).

bellet · 2017-09-07T15:56:51Z

@GaelVaroquaux you are OK with the solution provided by this PR or with the idea of adding an option to dump to file? In the first case we do not need to pickle anything, do we?

GaelVaroquaux · 2017-09-07T16:33:30Z

@bellet : I am OK with the solution provided by this PR. I would say that the issues raise by @jnothman need to be addressed, and a test needs to be added, and I would be +1 for merging.

jnothman · 2017-09-07T22:39:52Z

would it be more useful, to avoid keeping it all in memory, to be dumping models to disk?

…

On 8 Sep 2017 2:33 am, "Gael Varoquaux" ***@***.***> wrote: @bellet <https://github.com/bellet> : I am OK with the solution provided by this PR. I would say that the issues raise by @jnothman <https://github.com/jnothman> need to be addressed, and a test needs to be added, and I would be +1 for merging. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9686 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6x2YuvFvqB4-KRKQJn20C6i1Qmw3ks5sgBrdgaJpZM4PMHJx> .

GaelVaroquaux · 2017-09-07T22:44:32Z

would it be more useful, to avoid keeping it all in memory, to be dumping models to disk?

We really don't want to be coupling algorithms with persistence. It opens a bag of problems (such as files collision across several code paths / VM). If people really want to do that (and I sometimes want to), they should write the for loop themselves. It's not that hard.

jnothman · 2017-09-07T23:23:07Z

I would have just had users prescribe a filename format where the iteration index would be substituted in.

…

On 8 September 2017 at 08:44, Gael Varoquaux ***@***.***> wrote: > would it be more useful, to avoid keeping it all in memory, to be dumping models to disk? We really don't want to be coupling algorithms with persistence. It opens a bag of problems (such as files collision across several code paths / VM). If people really want to do that (and I sometimes want to), they should write the for loop themselves. It's not that hard. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9686 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6-t3aLYyoEvVf2Ti3Z908xb9M_4yks5sgHHRgaJpZM4PMHJx> .

bellet · 2017-09-13T16:28:18Z

OK I will go ahead with this solution and fix the issues you raised and we can then see who is OK with merging

bellet · 2017-09-18T20:32:15Z

I have addressed the issues raised by @jnothman (not sure whether my alternative to the nested ifs is neater...) and added a test. Comments welcome

bellet · 2017-09-20T14:56:03Z

I do not really understand why the code coverage fails. Unless I misunderstand the report it seems that all of my changes are covered...

bellet · 2017-09-22T07:01:37Z

@jnothman can you see what's wrong?

jnothman

I agree it's surprising that there's no coverage... perhaps that test is somehow not being run. Put an assert False in with your assertions just to check...?

jnothman · 2017-09-24T23:45:04Z

sklearn/model_selection/_validation.py

@@ -489,6 +511,8 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
        ret.extend([fit_time, score_time])
    if return_parameters:
        ret.append(parameters)
+    if return_estimator:
+        ret.append(estimator)


Apparently coverage is missing from this line!

I have added an assert False along with my assert_almost_equal on the estimator parameters and this makes the test fail, so they are run. Hence it doesn't make sense that this line is not covered, otherwise how can the test pass?

perhaps put the assert False here just to prove the coverage tool wrong? If it does not fail, you've got some investigation to do...

Thanks, just did this and the test now fails. Should I commit this for the sake of it?

How strange...

jnothman · 2017-10-17T04:24:47Z

Do you believe there's more to do here before full review? If not, relabel WIP -> MRG please.

bellet · 2017-10-17T06:26:59Z

I actually need to add the new option to the doc

jnothman

Otherwise LGTM

jnothman · 2017-10-17T06:45:30Z

sklearn/model_selection/_validation.py

@@ -140,6 +144,8 @@ def cross_validate(estimator, X, y=None, groups=None, scoring=None, cv=None,
                The time for scoring the estimator on the test set for each
                cv split. (Note time for scoring on the train set is not
                included even if ``return_train_score`` is set to ``True``
+            ``estimator``
+                The list of estimator objects for each cv split.


Drop "list of" for consistency

jnothman · 2017-10-17T06:49:42Z

sklearn/model_selection/_validation.py

    if return_train_score:
-        train_scores, test_scores, fit_times, score_times = zip(*scores)
+        train_scores = zipped_scores[0]


I think = zipped_scores.pop(0) and similar below will help simplify this logic.

bellet · 2017-10-17T07:54:23Z

Thanks for the comment, this indeed simplifies things a bit. I also briefly mentioned the new option in the doc.

jnothman · 2017-10-17T08:06:26Z

doc/modules/cross_validation.rst

@@ -196,6 +196,8 @@ following keys -
 for all the scorers. If train scores are not needed, this should be set to
 ``False`` explicitly.

+``return_estimator`` is set to ``False`` by default. When set to ``True``, it adds an ``estimator`` key containing the estimators fitted on each split.


Please limit lines to under 80 characters

I also think this is somewhat unnecessary. Or I'd state it more in terms of use case rather than API: "you can also retain to estimator fitted for each training set with return_estimator=True"

jnothman · 2017-10-17T08:10:27Z

sklearn/model_selection/_validation.py

        train_scores = _aggregate_score_dicts(train_scores)
+    if return_estimator:
+        (test_scores, fit_times, score_times,
+         fitted_estimators) = zipped_scores


I'd use pop here too and remove the else

jnothman · 2017-10-17T08:13:07Z

doc/modules/cross_validation.rst

@@ -196,6 +196,8 @@ following keys -
 for all the scorers. If train scores are not needed, this should be set to
 ``False`` explicitly.

+``return_estimator`` is set to ``False`` by default. When set to ``True``, it adds an ``estimator`` key containing the estimators fitted on each split.


I also think this is somewhat unnecessary. Or I'd state it more in terms of use case rather than API: "you can also retain to estimator fitted for each training set with return_estimator=True"

bellet · 2017-10-24T10:01:29Z

@GaelVaroquaux do you have additional comments? thanks!

amueller · 2017-10-24T21:23:05Z

travis is failing.

amueller

LGTM apart from minor nitpicks and doctest failures.

amueller · 2017-10-24T21:24:47Z

doc/modules/cross_validation.rst

@@ -182,7 +182,7 @@ The ``cross_validate`` function differs from ``cross_val_score`` in two ways -

 - It allows specifying multiple metrics for evaluation.

- It returns a dict containing training scores, fit-times and score-times in


optionally?

amueller · 2017-10-24T21:25:58Z

sklearn/model_selection/_validation.py

@@ -149,6 +153,8 @@ def cross_validate(estimator, X, y=None, groups=None, scoring=None, cv=None,
                The time for scoring the estimator on the test set for each
                cv split. (Note time for scoring on the train set is not
                included even if ``return_train_score`` is set to ``True``
+            ``estimator``
+                The estimator objects for each cv split.


if return_estimator is True.

bellet · 2017-10-26T11:28:29Z

@amueller for travis failure see above discussion #9686 (review) with @jnothman
I could not find an explanation for this behavior. The test passes on my machine for both Python 2 and 3.

jnothman · 2017-10-26T11:52:41Z

doc/modules/cross_validation.rst

@@ -227,8 +231,9 @@ Here is an example of ``cross_validate`` using a single metric::

    >>> scores = cross_validate(clf, iris.data, iris.target,
    ...                         scoring='precision_macro')


this should have a comma at the end

Oops, nice catch!

bellet · 2017-11-16T09:49:27Z

@amueller do you have other requests? thanks!

amueller · 2017-12-15T16:43:43Z

travis is failing because of doctests.

…6827-2

bellet · 2017-12-16T00:43:08Z

How do I locate which doctests fail? All tests pass when I run nosetests on sklearn/model_selection/_validation.py (except for a warning which is not related to this PR).

nosetests sklearn/model_selection/_validation.py  --with-doctest -v
Doctest: sklearn.model_selection._validation._aggregate_score_dicts ... ok
Doctest: sklearn.model_selection._validation.cross_val_predict ... ok
Doctest: sklearn.model_selection._validation.cross_val_score ... ok
Doctest: sklearn.model_selection._validation.cross_validate ... deprecation.py:122: FutureWarning: You are accessing a training score ('train_r2'), which will not be available by default any more in 0.21. If you need training scores, please set return_train_score=True
  warnings.warn(*warn_args, **warn_kwargs)
ok

----------------------------------------------------------------------
Ran 4 tests in 0.109s

OK

bellet · 2017-12-16T08:44:06Z

OK it seems it was only the documentation thing. All checks pass now @amueller

bellet · 2018-02-12T13:43:02Z

@amueller @jnothman @GaelVaroquaux anything else should be done here?

jnothman · 2018-02-27T11:05:07Z

Please add an Enhancements entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

jnothman · 2018-02-27T11:05:17Z

or maybe this is a New Feature. Otherwise, this is good to merge.

…6827-2

bellet · 2018-02-27T21:23:57Z

@jnothman done, thanks!

jnothman · 2018-02-27T22:33:07Z

Thanks @bellet!

add option to cross_validate to return the estimators fitted on each …

1b30589

…split

jnothman reviewed Sep 4, 2017

View reviewed changes

fixed variable name and nested loop and added a test

468556e

jnothman reviewed Sep 24, 2017

View reviewed changes

bellet changed the title ~~[WIP] add option to cross_validate to return estimators fitted on each split~~ [MRG] add option to cross_validate to return estimators fitted on each split Oct 17, 2017

bellet changed the title ~~[MRG] add option to cross_validate to return estimators fitted on each split~~ [WIP] add option to cross_validate to return estimators fitted on each split Oct 17, 2017

jnothman approved these changes Oct 17, 2017

View reviewed changes

bellet added 2 commits October 17, 2017 09:43

fixes suggested by jnothman

e690acd

mention return_estimator option in doc

4e65976

bellet changed the title ~~[WIP] add option to cross_validate to return estimators fitted on each split~~ [MRG] add option to cross_validate to return estimators fitted on each split Oct 17, 2017

jnothman approved these changes Oct 17, 2017

View reviewed changes

jnothman changed the title ~~[MRG] add option to cross_validate to return estimators fitted on each split~~ [MRG+1] add option to cross_validate to return estimators fitted on each split Oct 17, 2017

bellet and others added 2 commits October 17, 2017 10:24

fixes as suggested

b60cd89

Merge branch 'master' into i/6827-2

8e4ba61

amueller reviewed Oct 24, 2017

View reviewed changes

fixes suggested by amueller

87b52f0

jnothman reviewed Oct 26, 2017

View reviewed changes

comma fix

cc4165f

bellet added 3 commits December 15, 2017 16:06

right order of keys in doc

e6e54d7

missing apostrophe

f166c53

Merge branch 'master' of github.com:scikit-learn/scikit-learn into i/…

31215b5

…6827-2

bellet added 2 commits February 27, 2018 21:06

Merge branch 'master' of github.com:scikit-learn/scikit-learn into i/…

03fc79e

…6827-2

update whats new file

e4374a0

jnothman merged commit d9c2122 into scikit-learn:master Feb 27, 2018

bellet deleted the i/6827-2 branch February 27, 2018 23:54

		@@ -182,7 +182,7 @@ The ``cross_validate`` function differs from ``cross_val_score`` in two ways -

		- It allows specifying multiple metrics for evaluation.

		- It returns a dict containing training scores, fit-times and score-times in

		@@ -227,8 +231,9 @@ Here is an example of ``cross_validate`` using a single metric::

		>>> scores = cross_validate(clf, iris.data, iris.target,
		... scoring='precision_macro')

[MRG+1] add option to cross_validate to return estimators fitted on each split #9686

[MRG+1] add option to cross_validate to return estimators fitted on each split #9686

Conversation

bellet commented Sep 4, 2017 • edited Loading

Reference Issues

What does this implement/fix? Explain your changes.

Any other comments?

jnothman commented Sep 4, 2017 via email

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellet commented Sep 5, 2017

jnothman commented Sep 5, 2017

bellet commented Sep 5, 2017

GaelVaroquaux commented Sep 5, 2017 via email

bellet commented Sep 7, 2017

GaelVaroquaux commented Sep 7, 2017

jnothman commented Sep 7, 2017 via email

GaelVaroquaux commented Sep 7, 2017 via email

jnothman commented Sep 7, 2017 via email

bellet commented Sep 13, 2017

bellet commented Sep 18, 2017

bellet commented Sep 20, 2017 • edited Loading

bellet commented Sep 22, 2017

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellet Sep 29, 2017 • 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

jnothman commented Oct 17, 2017

bellet commented Oct 17, 2017

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellet commented Oct 17, 2017

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

bellet commented Oct 24, 2017

amueller commented Oct 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellet commented Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellet commented Nov 16, 2017

amueller commented Dec 15, 2017

bellet commented Dec 16, 2017 • edited Loading

bellet commented Dec 16, 2017 • edited Loading

bellet commented Feb 12, 2018

jnothman commented Feb 27, 2018

jnothman commented Feb 27, 2018

bellet commented Feb 27, 2018

jnothman commented Feb 27, 2018

bellet commented Sep 4, 2017 •

edited

Loading

bellet commented Sep 20, 2017 •

edited

Loading

bellet Sep 29, 2017 •

edited

Loading

bellet commented Oct 26, 2017 •

edited

Loading

bellet commented Dec 16, 2017 •

edited

Loading

bellet commented Dec 16, 2017 •

edited

Loading