[MRG+1] Rename CV params n_{folds,iter} to n_splits #7187

yenchenlin · 2016-08-13T10:53:14Z

Since now all CV splitters have get_n_splits(), we can rename n_{folds,iter} to n_splits in order to avoid confusion.

More detailed discussion can be found in #7169 .

Rename n_iter to n_splits
Rename n_ folds to n_splits

jnothman · 2016-08-13T11:07:07Z

sklearn/model_selection/tests/test_split.py

@@ -134,7 +134,7 @@ def test_cross_validator_with_default_params():
    n_unique_labels = 4
    n_folds = 2
    p = 2
-    n_iter = 10  # (the default value)
+    n_splits= 10  # (the default value)


jnothman · 2016-08-13T11:09:25Z

One question is whether we should be doing any deprecation... I think not, in this case.

jnothman · 2016-08-14T01:47:56Z

Test failures.

yenchenlin · 2016-08-14T01:58:45Z

~~Oh yeah, I submit this PR to ask question:~~

~~Should I modify old splitter in sklearn/cross_validation.py?~~
~~This line seems to use old splitter, and therefore results in error.~~

Update: Never mind, I was trapped 👋 .

jnothman · 2016-08-14T02:31:04Z

More test failures, though!

jnothman · 2016-08-14T02:50:00Z

Remaining failures are uses of n_folds in doc/tutorial/statistical_inference/model_selection.rst

yenchenlin · 2016-08-14T04:10:13Z

@jnothman @MechCoder CI is smiling 😄 , I think it's ready to be reviewed!

jnothman · 2016-08-14T13:27:20Z

examples/svm/plot_svm_scale_c.py

@@ -128,7 +128,7 @@
        # To get nice curve, we need a large number of iterations to
        # reduce the variance
        grid = GridSearchCV(clf, refit=False, param_grid=param_grid,
-                            cv=ShuffleSplit(train_size=train_size, n_iter=250,
+                            cv=ShuffleSplit(train_size=train_size, n_splits=250,


jnothman · 2016-08-14T13:41:46Z

Multiple PEP8 line length violations...

jnothman · 2016-08-14T13:44:13Z

sklearn/model_selection/tests/test_split.py

               "train_size=None)")
    ps_repr = "PredefinedSplit(test_fold=array([1, 1, 2, 2]))"

-    n_splits = [n_samples, comb(n_samples, p), n_folds, n_folds,
-                n_unique_labels, comb(n_unique_labels, p), n_iter, 2]
+    splits_cnts = [n_samples, comb(n_samples, p), n_splits, n_splits,


expected_n_splits or n_splits_expected might be better.

The issue is that cnt, apart from being an abbreviation we should try to avoid for clarity, means the same as n. So splits_cnt means the same as n_splits and so the naming tells us little of what it contains.

Another option is renaming the new n_splits to n_splits_param

Thanks for the elaboration, it's useful for me in the future 😄

jnothman · 2016-08-14T14:03:38Z

Otherwise LGTM

jnothman · 2016-08-14T14:10:31Z

Please draft a change to what's new, both in the Model Selection Enhancements section and in, perhaps, API Changes.

raghavrv · 2016-08-14T22:32:23Z

Yes this should go into model selection enhancements section. Otherwise with a whatsnew and reviews addressed this LGTM too.... Let's merge this! Thanks for the PR @yenchenlin

jnothman · 2016-08-15T23:40:07Z

doc/whats_new.rst

@@ -62,6 +62,16 @@ Model Selection Enhancements and API Changes
    the corresponding parameter is not applicable. Additionally a list of all
    the parameter dicts are stored at ``results_['params']``.

+  - **Renaming of `n_folds` and `n_iter` to `n_splits`**
+
+    The number of folds (i.e. the number of partitions of a dataset) is not 


I think this doesn't need so much motivation here, you just need to say something like "In the new model_selection module, some parameter names have changed: the n_folds parameter in x, y, z is now n_splits; the n_iter parameter in a, b, c is now n_splits". Please use :class: as appropriate.

yenchenlin · 2016-08-16T04:08:46Z

Comments addressed, please have a look!

jnothman · 2016-08-16T04:11:23Z

LGTM

raghavrv · 2016-08-16T14:24:40Z

doc/whats_new.rst

@@ -62,6 +62,16 @@ Model Selection Enhancements and API Changes
    the corresponding parameter is not applicable. Additionally a list of all
    the parameter dicts are stored at ``results_['params']``.

+  - **Renaming of ``n_folds`` and ``n_iter`` to ``n_splits``**
+
+    Some parameter names have changed: 


You could remove this line I think. You already have a heading...

I think this line is good.

I thought after the below suggested change, this is not needed...

Perhaps...? I'm unconvinced that this series of comments markedly contributes to the quality of the text in terms of convention, clarity or information structure.

raghavrv · 2016-08-16T14:32:05Z

Feel free to add a +1 once the comments are addressed. Ping @jnothman for merge once done...

raghavrv · 2016-08-16T15:16:11Z

@jnothman Would you like to merge it as such? LGTM...

yenchenlin · 2016-08-16T16:04:32Z

@raghavrv great thanks a lot for your review!
@jnothman comments addressed!

agramfort · 2016-08-16T19:19:21Z

Lgtm

@jnothman merge if you are happy

MechCoder · 2016-08-16T20:57:18Z

(Y)

amueller · 2016-08-18T22:31:00Z

aaaand my book broke ^^ glad it's not in print yet lol. I should do that release some time soom.

jnothman · 2016-08-18T22:36:52Z

hahahahaha. Glad you wrote it for the version where everything changes.

amueller · 2016-08-19T15:37:56Z

Well that's why I knew I couldn't do it for 0.17. But It would be great to release 0.18 within the next month (which I think is doable).

amueller · 2016-08-22T22:20:13Z

Shouldn't the default implementation of get_n_splits be return n_splits?

jnothman · 2016-08-22T23:48:35Z

I think it is better explicitly implemented...?

* Rename n_iter to n_splits * Fix bug * Fix examples * Add spaces * Rename n_folds to n_splits * Fix error * Fix doc * Fix doc * Fix example * Rename variables name * PEP8 * Fix error message * Add whats_new * Fix test * Fix doc * Fix doc * Make test clear

yenchenlin added 2 commits August 13, 2016 15:57

Rename n_iter to n_splits

12dc627

Fix bug

48a3905

jnothman reviewed Aug 13, 2016
View reviewed changes

yenchenlin added 3 commits August 13, 2016 23:28

Fix examples

94254d3

Add spaces

5110b08

Rename n_folds to n_splits

9349fdc

Fix error

ce034af

Fix doc

b63712b

yenchenlin added 2 commits August 14, 2016 10:58

Fix doc

806617d

Fix example

2869099

yenchenlin changed the title ~~[WIP] Rename CV params n_{folds,iter} to n_splits~~ [MRG] Rename CV params n_{folds,iter} to n_splits Aug 14, 2016

jnothman reviewed Aug 14, 2016
View reviewed changes

jnothman mentioned this pull request Aug 14, 2016

[MRG+2?] Add homogeneous time series cross validation #6586

Merged

yenchenlin force-pushed the rename-model-selection branch from dd2ff8e to e216af5 Compare August 15, 2016 12:19

Rename variables name

3406ad8

jnothman reviewed Aug 15, 2016
View reviewed changes

Fix doc

ce6d515

jnothman changed the title ~~[MRG] Rename CV params n_{folds,iter} to n_splits~~ [MRG+1] Rename CV params n_{folds,iter} to n_splits Aug 16, 2016

raghavrv reviewed Aug 16, 2016
View reviewed changes

yenchenlin added 2 commits August 17, 2016 00:01

Fix doc

85c6b6a

Make test clear

ab55ba4

MechCoder merged commit 42120e5 into scikit-learn:master Aug 16, 2016

yenchenlin mentioned this pull request Aug 24, 2016

Introduce a mixin to reactor get_n_splits() in model_selection module #7235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Rename CV params n_{folds,iter} to n_splits #7187

[MRG+1] Rename CV params n_{folds,iter} to n_splits #7187

yenchenlin commented Aug 13, 2016 •

edited

Loading

jnothman Aug 13, 2016

jnothman commented Aug 13, 2016

jnothman commented Aug 14, 2016

yenchenlin commented Aug 14, 2016 •

edited

Loading

jnothman commented Aug 14, 2016

jnothman commented Aug 14, 2016

yenchenlin commented Aug 14, 2016

jnothman Aug 14, 2016

jnothman commented Aug 14, 2016 •

edited

Loading

jnothman Aug 14, 2016

jnothman Aug 14, 2016

raghavrv Aug 14, 2016

yenchenlin Aug 15, 2016

jnothman commented Aug 14, 2016

jnothman commented Aug 14, 2016

raghavrv commented Aug 14, 2016

jnothman Aug 15, 2016

yenchenlin commented Aug 16, 2016

jnothman commented Aug 16, 2016

raghavrv Aug 16, 2016

jnothman Aug 16, 2016

raghavrv Aug 16, 2016 •

edited

Loading

jnothman Aug 16, 2016

raghavrv commented Aug 16, 2016

raghavrv commented Aug 16, 2016 •

edited

Loading

yenchenlin commented Aug 16, 2016

agramfort commented Aug 16, 2016

MechCoder commented Aug 16, 2016

amueller commented Aug 18, 2016

jnothman commented Aug 18, 2016

amueller commented Aug 19, 2016

amueller commented Aug 22, 2016

jnothman commented Aug 22, 2016

[MRG+1] Rename CV params n_{folds,iter} to n_splits #7187

[MRG+1] Rename CV params n_{folds,iter} to n_splits #7187

Conversation

yenchenlin commented Aug 13, 2016 • edited Loading

Choose a reason for hiding this comment

jnothman commented Aug 13, 2016

jnothman commented Aug 14, 2016

yenchenlin commented Aug 14, 2016 • edited Loading

jnothman commented Aug 14, 2016

jnothman commented Aug 14, 2016

yenchenlin commented Aug 14, 2016

Choose a reason for hiding this comment

jnothman commented Aug 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

Choose a reason for hiding this comment

jnothman commented Aug 14, 2016

jnothman commented Aug 14, 2016

raghavrv commented Aug 14, 2016

Choose a reason for hiding this comment

yenchenlin commented Aug 16, 2016

jnothman commented Aug 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv Aug 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv commented Aug 16, 2016

raghavrv commented Aug 16, 2016 • edited Loading

yenchenlin commented Aug 16, 2016

agramfort commented Aug 16, 2016

MechCoder commented Aug 16, 2016

amueller commented Aug 18, 2016

jnothman commented Aug 18, 2016

amueller commented Aug 19, 2016

amueller commented Aug 22, 2016

jnothman commented Aug 22, 2016

yenchenlin commented Aug 13, 2016 •

edited

Loading

yenchenlin commented Aug 14, 2016 •

edited

Loading

jnothman commented Aug 14, 2016 •

edited

Loading

raghavrv Aug 16, 2016 •

edited

Loading

raghavrv commented Aug 16, 2016 •

edited

Loading