Please don't modify cross_validation.

On 20 September 2016 at 13:35, Nelson Liu notifications@github.com wrote:

Reference Issue

Fixes #5948 #5948
What does this implement/fix? Explain your changes.

Changes the default behavior of train_size and test_size in splitters.
The defaults for both parameters are now None

• if train_size and test_size are both None, train_size is set to 0.1
  and test_size is set to0.9`
• if only one of train_size or test_size are set, then the value of
  the unset parameter is n_samples - set_parameter (the complement)
• If both are explicitly set, check that they are equal to or less
  than 0 and respect that the user wants to subsample the dataset

Any other comments?

I'm aware that the cross_validation module is being deprecated; does this
mean that I shouldn't add these changes to it and only to model_selection?
TODO

• Edit documentation to clearly explain these new changes

---

You can view, comment on, or merge this pull request online at:

#7459
Commit Summary

• edit train/test_size default behavior

File Changes

• M sklearn/cross_validation.py
  https://github.com/scikit-learn/scikit-learn/pull/7459/files#diff-0
  (41)
• M sklearn/model_selection/_split.py
  https://github.com/scikit-learn/scikit-learn/pull/7459/files#diff-1
  (54)
• M sklearn/model_selection/tests/test_split.py
  https://github.com/scikit-learn/scikit-learn/pull/7459/files#diff-2
  (15)
• M sklearn/tests/test_cross_validation.py
  https://github.com/scikit-learn/scikit-learn/pull/7459/files#diff-3
  (36)

Patch Links:

• https://github.com/scikit-learn/scikit-learn/pull/7459.patch
• https://github.com/scikit-learn/scikit-learn/pull/7459.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7459, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAEz6w0JaHZaFQeJaKrIaip96r33SnH0ks5qr1SAgaJpZM4KBMM_
.

Please don't modify cross_validation.

ok, thanks.

This needs a rebase on master to fix the conflicts.

This needs a rebase on master to fix the conflicts.

Done, thanks for letting me know. Didn't notice / realize someone else was working on the file as well :)

this is ready to be looked at, missing a whatsnew though (I'd like to verify the current behavior is correct before writing it). Among the changes discussed earlier, GroupShuffleSplit has test_size set to a default of 0.2. This PR changes it to 0.1 to match all the other splitters.

I expect the docs of train_test_split also need a change. And you can't change the default behavior of GroupShuffleSplit without a deprecation. We could have done that in 0.18, but now it's too late :-/

right, sorry for omitting that @amueller . I've actually never "deprecated" by changing the default parameter of a function before, would you mind pointing me to an example / explaining how to do it? It seems to be missing in the contributing docs as well, so I'd be happy to add it there.

I'm not sure we should actually change the default from 0.2 to 0.1 in that case.

For reference, the deprecation documentation is here:
http://scikit-learn.org/dev/developers/contributing.html#deprecation

I'm always kinda confused by the fact that the "Contributing" link on the help page doesn't go to contributing, but to developer....

hmm, would

            warnings.warn("The default value of train_size parameter was "
                          "changed from 0.2 to None, which changes the "
                          "default (train_size, test_size)"
                          "from (0.2, 0.8) to (0.1, 0.9) "
                          "in version 0.19", DeprecationWarning)

be clear? I feel like there's a better way to word it...

Usually we just say "The default value of (..) will change from .. to .. in version 0.20" and the None is really just a weird implementation detail

@amueller we also need a deprecation for train_test_split right? Considering the defaults will change from 0.25,0.75 to 0.1,0.9 in version 0.21

also, by the docs of train_test_split need a change, do you mean the docstrings or the examples / tutorial material?

also I edited the default parameters taken by the __init__ of GroupShuffleSplit to take test_size=None and train_size=None, allowing me to throw the deprecation warning and then accordingly set the previous defaults (test_size=0.2 and train_size-0.8). Should I just not have touched this?

We should not change the defaults of train_test_split. We should use the same behavior, though. And I meant the docstrings need to document the new way train_size and test_size interact.

There are a couple of errors in your issue description:

• if train_size and test_size are both None, train_size is set to 0.1 and test_size is set to 0.9

you mean the opposite

• If both are explicitly set, check that they are equal to or less than 0 and respect that the user wants to subsample the dataset

I think you mean less than n_samples or less than 1.

Now that model_selection is released we need to be aware of backwards compatibility issues. For now, if a user sets train_size without setting test_size (and train_size+test_size > 1 or n_samples) the best we can do is warn that the behaviour will change in a future release, IMO.

We should not change the defaults of train_test_split. We should use the same behavior, though. And I meant the docstrings need to document the new way train_size and test_size interact.

Makes sense, I've reverted the change and updated the description accordingly.

There are a couple of errors in your issue description:

Indeed, thanks for catching that.

Now that model_selection is released we need to be aware of backwards compatibility issues. For now, if a user sets train_size without setting test_size (and train_size+test_size > 1 or n_samples) the best we can do is warn that the behaviour will change in a future release, IMO.

To clarify --- we should not implement the changes to the behavior when setting train_size without test_size, but throw a warning instead saying that it will change in future release (sort of like a deprecation)? Also, how is there a case that train_size+test_size > 1 or n_samples? Sorry if these questions have obvious answers.

I'm not now sure about the details of my comment. Point is that all behaviours need to remain identical, except perhaps for a warning or extra user-specified parameters/values handled. I need to take a proper look at what's changing here to give more precise feedback but there's a huge load of issues for me to skim right now...

I hope to look at this soon, @nelson-liu. In the meantime, resolving merge conflicts with master wouldn't hurt.

@jnothman feel free to hassle me about this, too ;)

Rebased on master to fix conflicts.

thanks @lesteve! not sure if this is ready to be looked at actually, I don't think i've addressed @jnothman 's latest comments wrt keeping behaviors identical...

@jnothman - let me know if i understood what to do correctly. The idea is that the current behavior shouldn't change, and thus this PR should just add the functions that detect when a user invokes behavior that will change in the future (test_size default, unspecified value, train_size some specified value) and warn them that the behavior of the code will change in 0.21. Then when 0.20dev comes along, we'll actually implement the new behavior?

If this is the case, I reverted everything to ensure that it maintains the current behavior and just warns the user appropriately.

but this seems a little too narrative?

Maybe you can put a short snippet and mention the values of test_size/train_size in both 0.20 and 0.21. By the way it may be worth creating an issue to make sure we remove the warnings in 0.21 and we add an entry in the 0.21 changelog in the "API changes" section ...

Sufficient what's new:

In version 0.21, the default behavior of splitters that use the test_size and train_size parameter will change, such that specifying train_size alone will cause test_size to be the remainder.

Codecov Report

Merging #7459 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7459      +/-   ##
==========================================
+ Coverage   95.48%   95.48%   +<.01%     
==========================================
  Files         342      342              
  Lines       61013    61033      +20     
==========================================
+ Hits        58259    58279      +20     
  Misses       2754     2754

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 919b4a8...fd49cb9. Read the comment docs.

Thanks @nelson-liu

sorry for letting this languish @jnothman , and thanks for taking it into your own hands to patch it up.

So what actually changed here, in practice? If the user did nothing they would have 0.75/0.25 before, and it looks like that is still the case.

So what actually changed here, in practice?

No breaking behavior has been implemented in version 0.19 (hence the FutureWarning). In version 0.21, if you specify train_size without specifying test_size, it will automatically set test_size = 1 - train_size

If the user did nothing they would have 0.75/0.25 before, and it looks like that is still the case.

Yes, that is correct.

Right, so what I'm confused by is it seems like that's already the behavior. From the doc: "If None, the value is set to the complement of the train size."

ah, test_size is not None by default (it's now a sentinel string "default", and was the float 0.1 in version 0.18). If you explicitly pass None to it, then it will complement the train size. This is the case in both 0.18 and 0.19.

Does that help clarify?

That helps, although I would have thought the default was the float 0.25 since the docs also say, "By default, the value is set to 0.25." Was it 0.1 only in version 0.18?

Also, note that the defaults are different for different splitting functions/classes (e.g. train_test_split does have a default of 0.25 instead of 0.1).

edit: note that the train_test_split default test_size is 0.25 only if train_size and test_size are both none (which is their default value, unlike the CVSplitters).

But you're saying it was 0.1 for one version?

The default test_size for BaseSplitter has been 0.1 for at least 0.17 and 0.18 (and possibly longer).

As far as i can remember, the default test_size for train_test_split has been 0.25 (it could have been changed from 0.1, though -- you can probably find out by looking at the release history)

In addition: I'm not sure what objects you're referring to by "it" --- the default behavior of the Splitter classes and train_test_split are different (and this PR touches both, so unsure which you're referring to). Hopefully I answered your question though.

Ok thanks. I just wanted to make sure the defaults for train_test_split hadn't changed recently because if so it would affect my models. It doesn't sound like it, so I'm satisfied. I guess this is a good example of the wisdom of always specifying parameters rather than relying on defaults.