Fixes #11129: Change cv default to 5 by shaz13 · Pull Request #11139 · scikit-learn/scikit-learn

shaz13 · 2018-05-25T13:59:00Z

shaz13 · 2018-05-25T13:59:43Z

Upcoming commits with deprecation warning in progress.

amueller · 2018-05-26T00:47:13Z

Please check out http://scikit-learn.org/dev/developers/contributing.html#deprecation

amueller · 2018-05-26T14:56:01Z

sklearn/model_selection/_split.py

        If None, the random number generator is the RandomState instance used
        by `np.random`. Used when ``shuffle`` == True.

+    .. deprecated:: 0.20


this should be "versionchanged" I think? Maybe? The parameter is not deprecated, only the default value changed.

amueller · 2018-05-26T14:57:04Z

thanks. Can you please check the travis failure?
Also, you should make sure that our test suite actually doesn't throw these warnings anywhere. Either we should catch them, or we can specify the value explicitly to avoid the warning.

Modified the code. Added suitable code for test coverage

shaz13 · 2018-05-27T18:39:18Z

Update: Few more tests left to cover. Commit will be pushed by tomorrow. Thanks!

jnothman

Rather than catching the warnings, we should make sure to pass cv explicitly in all docs and tests

jnothman · 2018-05-27T23:38:48Z

sklearn/model_selection/_split.py

        by `np.random`. Used when ``shuffle`` == True.

+    .. versionchanged:: 0.20
+    The default value ``n_splits=3`` is deprecated in version 0.20 and will 


This must be indented

Missed this in hurry. Done. Thanks! 👍

jnothman · 2018-05-27T23:44:31Z

sklearn/model_selection/_split.py

        if self.shuffle:
            check_random_state(self.random_state).shuffle(indices)

+        if self.n_splits == 3:


This is the wrong place to warn. It should happen in check_cv. It should also not warn if the user explicitly sets cv to 3

Moved it to check_cv. The tests passed locally.

shaz13 · 2018-05-29T08:23:15Z

@amueller @jnothman All checks have passed. Please take a look!

qinhanmin2014 · 2018-05-29T09:12:29Z

Should this be a FutureWarning instead of DeprecationWarning? We're not deprecating anything and I recall there's similar discussion elsewhere (e.g., #10331 (comment))

urvang96 · 2018-05-29T11:11:18Z

sklearn/model_selection/_split.py

+    warnings.warn("The default value of n_splits=3 is deprecated"
+                  " in version 0.20 and will be changed to "
+                  "n_splits=5 in version 0.22", DeprecationWarning)
    if cv is None:


I guess the warning should only be displayed if cv is None, as cv will take the default value in that case.

Done. Thanks!

Replaced Deprecation Warnings with Future Warnings

lasagnaman · 2018-06-15T15:52:51Z

sklearn/model_selection/_split.py

        splits via the ``split`` method.
    """
    if cv is None:
+        warnings.warn("The default value of n_splits=3 is deprecated"


The default value of cv on line 1866 should be cv=None, otherwise we will never enter this block.

Done. Thanks!

maybe pass an explicit value for cv?

jnothman · 2018-06-25T08:02:02Z

sklearn/model_selection/tests/test_validation.py

            param_range=param_range, cv=2
        )
-    if len(w) > 0:
+    if len(w) > 1:


I think this is too loose. You could test != 1, with a comment on why you should expect 1 warning, or, better, you can remove the expected warning from w

jnothman · 2018-06-25T08:02:18Z

sklearn/linear_model/tests/test_least_angle.py

        lars_cv = linear_model.LassoLarsCV(max_iter=5)
        lars_cv.fit(X, y)
-    assert_true(len(w) == 0)
+    assert_true(len(w) >= 0)


No. See below

jnothman · 2018-06-26T07:55:24Z

sklearn/model_selection/tests/test_validation.py

-        result[val] = assert_no_warnings(cross_validate, estimator, X, y,
-                                         return_train_score=val)
-
+        result[val] = assert_warns_message(FutureWarning, msg_nsplit,


Firstly this needs to assert that no other warnings are issued. Secondly it needs to be clear to the developer tasked with completing this deprecation in a couple of years' time what it used to be asserting, i.e. that it was asserting an absence of warnings.t

Thirdly, the right solution is actually to pass an explicit value for cv here. Sorry I didn't notice that before.

qinhanmin2014

ping @shaz13 are you still working on it?

qinhanmin2014 · 2018-07-09T02:17:09Z

doc/whats_new/v0.20.rst


 Model evaluation and meta-estimators

+- The default of ``n_splits`` parameter of :class:`model_selection.KFold` is


We have a API changes summary section for it. Also, I think we need a reason for the API change. See e.g.,

The default value of ``gamma`` parameter of :class:`svm.SVC`, :class:`~svm.NuSVC`, :class:`~svm.SVR`, :class:`~svm.NuSVR`, :class:`~svm.OneClassSVM` will change from ``'auto'`` to ``'scale'`` in version 0.22 to account better for unscaled features. :issue:`8361` by :user:`Gaurav Dhingra <gxyd>` and :user:`Ting Neo <neokt>`.

qinhanmin2014 · 2018-07-09T02:17:53Z

sklearn/model_selection/_split.py

        splits via the ``split`` method.
    """
    if cv is None:
+        warnings.warn("The default value of n_splits=3 is deprecated"


maybe pass an explicit value for cv?

qinhanmin2014 · 2018-07-09T02:20:24Z

sklearn/model_selection/tests/test_search.py

                  RandomizedSearchCV(LinearSVC(random_state=0), grid,
                                     n_iter=2, iid=False)]
-
+    msg_nsplit = ("The default value of n_splits=3 is deprecated in "


I don't like the message here. It would be better if you can refer to merged PRs so that users can get similar messages for similar issues.

qinhanmin2014 · 2018-07-09T02:20:58Z

sklearn/model_selection/tests/test_search.py

            fit_func = ignore_warnings(estimator.fit,
                                       category=ConvergenceWarning)
-            result[val] = assert_no_warnings(fit_func, X, y).cv_results_
+            result[val] = assert_warns_message(FutureWarning, msg_nsplit,


maybe pass an explicit value for cv?

qinhanmin2014 · 2018-07-09T02:21:10Z

sklearn/model_selection/tests/test_search.py

    grid = GridSearchCV(SVC(gamma='scale'), param_grid={'C': [1]}, cv=3)
    # no warning with equally sized test sets
    assert_no_warnings(grid.fit, X, y)
-


unrelated change

shaz13 · 2018-07-09T05:24:07Z

ping @qinhanmin2014. Unfortunately, I cannot contribute to opensource source code until I am in a contract with my employer. Please feel free to reassign the issue to another contributor. Thanks!

qinhanmin2014 · 2018-07-09T07:17:28Z

Thanks @shaz13 for your great work so far :)

wenhaoz-fengcai · 2018-07-15T04:19:55Z

@qinhanmin2014 Should I open up a new PR if I want to take over this fix?

qinhanmin2014 · 2018-07-15T08:28:47Z

@jiaowoshabi You should open another PR :)
Contributions are always welcomed, but I just want to clarify that the issue is difficult and current PR only solves a small part of it.
See #11129 (comment)

aboucaud · 2018-07-16T14:21:07Z

@jiaowoshabi are you already working on it ? Otherwise I'd like to take over this PR for the current sprint.

wenhaoz-fengcai · 2018-07-16T16:50:31Z

@aboucaud I'm not working on this PR. Go ahead on your fix

aboucaud · 2018-07-19T12:51:53Z

@amueller @jnothman #11557 has been merged so this PR can be closed now.

Change of default cv from 3 to 5

009267a

Added deprecation warnings

e683777

Init corrections

646cc3d

amueller reviewed May 26, 2018

View reviewed changes

Addressed review comments

3fe6f32

Modified the code. Added suitable code for test coverage

jnothman reviewed May 27, 2018

View reviewed changes

shaz13 added 3 commits May 28, 2018 23:01

Addressed tests. Moved warning to chec_cv

6143eed

Flake8 corrections and few other tests

ee65184

Modified assertion value

66378db

urvang96 reviewed May 29, 2018

View reviewed changes

shaz13 added 4 commits May 29, 2018 21:58

Addressed Review Comments (2)

929c900

Replaced Deprecation Warnings with Future Warnings

Removed unnecessary test conditions

3bf45e0

init

b41e503

Update test_least_angle.py

64a837b

lasagnaman reviewed Jun 15, 2018

View reviewed changes

Updated default to None

2d99fe8

jnothman reviewed Jun 25, 2018

View reviewed changes

shaz13 added 3 commits June 25, 2018 16:27

Addressed comments

fdb34d8

init

bc26d71

init(2)

809920d

jnothman reviewed Jun 26, 2018

View reviewed changes

qinhanmin2014 reviewed Jul 9, 2018

View reviewed changes

qinhanmin2014 mentioned this pull request Jul 9, 2018

Change cv default to 5 #11129

Closed

qinhanmin2014 added Stalled help wanted labels Jul 9, 2018

qinhanmin2014 mentioned this pull request Jul 10, 2018

[MRG+2] ENH Passthrough DataFrame in FunctionTransformer #11043

Merged

3 tasks

aboucaud mentioned this pull request Jul 16, 2018

[MRG+1] Change CV defaults to 5 #11557

Merged

qinhanmin2014 closed this Jul 19, 2018

shaz13 deleted the kfold-cv-default-5 branch September 16, 2018 11:49


		Model evaluation and meta-estimators

		- The default of ``n_splits`` parameter of :class:`model_selection.KFold` is

Uh oh!

Conversation

shaz13 commented May 25, 2018

Uh oh!

shaz13 commented May 25, 2018

Uh oh!

amueller commented May 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented May 26, 2018

Uh oh!

shaz13 commented May 27, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaz13 commented May 29, 2018

Uh oh!

qinhanmin2014 commented May 29, 2018

Uh oh!

urvang96 May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaz13 commented Jul 9, 2018

Uh oh!

qinhanmin2014 commented Jul 9, 2018

Uh oh!

wenhaoz-fengcai commented Jul 15, 2018

Uh oh!

qinhanmin2014 commented Jul 15, 2018

Uh oh!

aboucaud commented Jul 16, 2018

Uh oh!

wenhaoz-fengcai commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aboucaud commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

urvang96 May 29, 2018 •

edited

Loading

wenhaoz-fengcai commented Jul 16, 2018 •

edited

Loading