[RFC] Changes to model_selection? #5053

amueller · 2015-07-29T20:41:59Z

Once #4294 is merged, we might want to think about changing some of the api / default behaviors of the contents of this module.

There are several breaking changes that I'd like to make, and this might or might not be a good opportunity:

changing the default n_folds to 5, as it is already in some of the cv objects (3 seems really low).
rename grid_scores_ to be consistent with RandomizedSerachCV
possibly redo the _CVScoreTuple [needs to be done for multiple metrics I think]
rename labels to groups (?) in the cv iterators.
rename the p parameter in LeavePOut

was there anything else? ping @rvraghav93

The text was updated successfully, but these errors were encountered:

raghavrv · 2015-08-02T14:22:41Z

(I'm checking the entries as soon as there is an issue/PR for the same)

MAJOR

Rename labels to groups? - (Merged [MRG+1] ENH/MNT Rename labels --> groups #6660)
As an addition to the "rename labels to groups" we also need to know if we need to provide separate labels for permutation and cv (when cv is LOLO / LPLO) in permutation_test_score
Discussions - [MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294 (comment) - [MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294 (comment) - [MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294 (comment)
Should stratified become a constructor parameter? (+1 from @jnothman @mblondel @amueller)
(Also refer extend StratifiedKFold to float for regression #4757 for discussion on stratification in the case of regression)

~~General tests for CV objects?~~

MODERATE

changing the default n_folds to 5, as it is already in some of the cv objects (3 seems really low). (Discarded because it was difficult to deprecate/modify all the cv param defaults in estimators?)
Rename grid_scores_ to be consistent with RandomizedSerachCV ([MRG+3] ENH Restructure grid_scores_ into a di 8000 ct of 1D (numpy) (masked) arrays that can be imported into pandas as a DataFrame. #6697)
Redo the _CVScoreTuple ([MRG+3] ENH Restructure grid_scores_ into a dict of 1D (numpy) (masked) arrays that can be imported into pandas as a DataFrame. #6697)
Rename the p parameter in LeavePOut - (Discarded)
Whether _permutation_test_score can be replaced by cross_val_score? (check 8cfebf2)
Add a test for fit_grid_point -
Test the _CVIterableWrapper better?? -
Move the param_grid validation inside __iter__ -
Currently cross_val_score does not have an option to return params... (but its helper _fit_and_score has). Do we need to provision that? See [MRG + 2] ENH Allow cross_val_score, GridSearchCV et al. to evaluate on multiple metrics #7388 (comment)

MINOR

Use isinstance(float) instead of dtype.kind == 'f'
test_check_is_permutation should be stronger - Refer [MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294 (comment)
Move the *CV consts to end of _split and use it in tests - [MRG+1] MNT remove ALL_CVS and LABEL_CVS from _validation.py #6517

RFC

Moved from #5569 to here

Do we need to pass y to check_cv in LogisticRegressionCV (fit method) - @amueller 's comment - Do we need y? I guess to be future proof in case we implement multi-label LR? - [MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294 (comment) - [MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294 (comment)
n_labels is ambiguous. How about n_labels_out? - Ping @amueller

jnothman · 2015-08-03T01:09:56Z

Defaulting n_folds to 5 is hard because it affects all *CV classes, not just things in model_selection.

rename grid_scores_ to be consistent with RandomizedSerachCV

This was postponed until we have a solution for multiple metrics and/or training scores, which may mandate changing the format, too: namedtuples are user-friendly but inflexible to versioning.

Having a clearer delineation of "labels" and "groups" would certainly be nice...

jnothman · 2015-08-03T07:31:29Z

In new style CV splitters, the constructor no longer differs between stratified and unstratified splitters. Should stratified become a constructor parameter so that we are not proliferating pairs of classes with parallel APIs?

amueller · 2015-08-03T15:58:54Z

True, changing n_folds is problematic :-/
For the stratified option: That might be interesting. I think we should also think about how this relates to stratifying in the regression case, see the discussion at #4757.

mblondel · 2015-08-05T15:12:48Z

Should stratified become a constructor parameter so that we are not proliferating pairs of classes with parallel APIs?

+1

jnothman · 2015-08-06T02:35:59Z

A minor change that could be included directly in #4294: drop grid_search.fit_grid_point.

jnothman · 2015-08-06T02:37:11Z

Half kidding: Is now also the time to rename get_params and set_params to get_config (as in keras) and set_config so as to avoid the confusion of parameters vs hyperparameters etc??

amueller · 2015-08-06T17:38:24Z

is there a confusion about that? I wasn't aware.

jnothman · 2015-08-07T03:15:51Z

Well, the confusion is only when discussing the models, rather than being an inherent API problem. It's irrelevant; it's a silly thing to change, certainly as long as we call function/constructor arguments "parameters".

raghavrv · 2015-09-13T04:35:53Z

@amueller @vene @jnothman sorry for going back and forth (- wrt to a discussion at gitter) on this, but how does grouping LabelKFold and KFold sound to you?

i.e LabelKFold === KFold.split(X, y, labels) and KFold === KFold.split(X, y, labels=None)? (similarly for ShuffleSplit? )

Also if this sounds okay... we should think about how StratifiedKFold with labels should be considered? (This is in reference to Joel's comment on combining KFold and StratifiedKFold like KFold(stratify=False))

So after that is done what would KFold(stratify=True, n_folds=5).split(X, y, labels=some_label) mean? Could it mean we stratify using y and use labels to make disjoint sets (is that even possible?)

Or would it be better to simply keep it as such as a separate class as it is right now?

jnothman · 2015-09-13T06:20:12Z

I think a separate class. It's not just an option.

On 13 September 2015 at 14:35, Raghav R V notifications@github.com wrote:

@amueller https://github.com/amueller @vene https://github.com/vene
@jnothman https://github.com/jnothman sorry for going back and forth (-
wrt to a discussion at gitter) on this, but how does grouping LabelKFold
and KFold sound to you?

i.e LabelKFold === KFold.split(X, y, labels) and KFold === KFold.split(X,
y, labels=None)? (similarly for ShuffleSplit? )

Also if this sounds okay... we should think about how StratifiedKFold
with labels should be considered? (This is in reference to Joel's comment
on combining KFold and StratifiedKFold like KFold(stratify=False))

So after that is done what would KFold(stratify=True, n_folds=5).split(X,
y, labels=some_label) mean? Could it mean we stratify using y and use
labels to make disjoint sets (is that even possible?)

Or would it be better to simply keep it as such as a separate class as it
is right now?

—
Reply to this email directly or view it on GitHub
#5053 (comment)
.

amueller · 2015-09-13T15:52:36Z

I also think a separate class would be good, if nothing else for visibility.

raghavrv · 2015-09-13T23:08:33Z

Okay! Thanks!

raghavrv · 2016-04-12T11:16:40Z

rename labels to groups (?) in the cv iterators.

Does this mean LabelKFold should be renamed to GroupedKFold?

@jnothman @amueller @vene

raghavrv · 2016-04-12T11:31:18Z

Also could we rename p to p_samples?

raghavrv · 2016-04-12T11:31:58Z

or n_test_samples or n_test?

jnothman · 2016-04-13T12:56:36Z

Personally, I much prefer GroupedKFold over LabelKFold as a description.

jnothman · 2016-04-13T12:57:57Z

I think leave p alone. It's a bit weird that KFold doesn't just have a parameter k.

raghavrv · 2016-04-13T12:59:57Z

I think leave p alone.

Pun intended? ;)

jnothman · 2016-04-13T13:04:36Z

I'm missing the pun. And I make dad jokes all the time.

On 13 April 2016 at 22:59, Raghav R V notifications@github.com wrote:

I think leave p alone.

Pun intended? ;)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5053 (comment)

raghavrv · 2016-04-13T13:06:18Z

Nevermind ;) (I thought LeavePOut --> Leave p alone :P )

raghavrv · Apr 14, 2016

BTW on a similar note should we also rename "Leave*LabelOut" --> "Leave*GroupOut"?

GaelVaroquaux · 2016-04-14T12:55:52Z

Also could we rename p to p_samples?

n_samples? I don't understand p_samples? Or maybe n_features?

GaelVaroquaux · 2016-04-14T12:57:05Z

Personally, I much prefer GroupedKFold over LabelKFold as a description.

But we shouldn't be renaming things just for cosmetics (consistency is a
valid argument, though). It uses a lot of energy both for our developers
and our users.

raghavrv · 2016-04-14T13:06:53Z

But we shouldn't be renaming things just for cosmetics (consistency is a valid argument, though)

I think the motivation for renaming labels --> groups is to demarcate clearly what we mean by labels. (As labels can also mean the target or class labels). Thanks for the comment! Could you also express your views at #6660 please?

vene · 2016-04-14T14:03:28Z

That's my view as well. labels was a weird, overloaded word in this context. I was personally very confused about what LeavePLabelOut did for a while, because I was thinking of y as labels. I thought it's some sort of zero-shot learning evaluation.

Groups is much better; though there could be confusion with, say, GroupLasso.

Such breakage is annoying on two accounts. (a) user code will need to change: but this will happen anyway because of the move to model_selection. (b) books becoming out of date, as @GaelVaroquaux points out.

Now, my first impulse is to say that (b) is also moot because of model_selection but it's not that simple. It's easier to realize that cross_validation.LeavePLabelOut became model_selection.LeavePLabelOut rather than if it becomes model_selection.LeavePGroupOut.

Still, I think that if you at least know what your intention is, and understand the word "Group", the latter can be easily identified as relevant when tabbing through the imports.

(In light of this, I would have preferred LabelKFold to be KFoldLabels so that it would be easy to type KFold[tab] and find KFoldGroups instead...)

raghavrv · 2016-04-14T14:18:12Z

@vene Could you copy the comment to #6660 please?? :)

ogrisel · 2016-09-11T17:16:11Z

Re-opening as #6660 only fixed the part on renaming labels to groups.

jnothman · 2016-09-11T23:19:50Z

I don't think anything remains here that needs to be changed before the release of model_selection. I think anything we want to consider changing can be moved to its own specific issue and this one closed.

raghavrv · 2016-09-11T23:27:27Z

+1 I'll do that tomorrow!

amueller · 2016-09-12T16:36:45Z

I think this issue can be closed. @raghavrv are there any urgent things you still see?

raghavrv · 2016-09-12T16:41:53Z

Few more tests that I was planning to raise as a PR soon... Otherwise this can be closed.

raghavrv · 2016-09-13T14:41:26Z

F438 @amueller Do we want to remove _permutation_test_score? I tried doing that here at this commit - 8cfebf2

It seems to add one more line and some code complexity. I don't feel it's worth it. Reviews on the commit and a quick comment on whether we need that as a PR?

vene · 2016-09-13T15:27:09Z

I think I prefer the version with the auxiliary function, it's much more readable to me at least.

raghavrv · 2016-09-13T15:27:36Z

Yes. Thanks for the comment!

amueller · 2016-09-13T16:56:06Z

why is groups passed and not used?

raghavrv · 2016-09-13T17:09:26Z

I am sorry... I don't get you. groups is used for both cv and shuffling... (It's passed to _shuffle to shuffle the target and to split for group cvs)

amueller · 2016-09-13T17:14:28Z

sorry lack of coffee

raghavrv · 2016-09-13T21:37:20Z

I think it can be closed now... The other comments are either addressed / have PRs for them. @amueller

raghavrv mentioned this issue Aug 2, 2015

[MRG+1] Make cross-validators data independent + Reorganize grid_search, cross_validation and learning_curve into model_selection #4294

Merged

24 tasks

raghavrv mentioned this issue Oct 23, 2015

[WIP] ENH Make stratified a parameter and conflate all the stratified/non-stratified cross-validator class pairs. #5569

Closed

3 tasks

raghavrv mentioned this issue Feb 8, 2016

[MRG+1] MAINT Reuse unaltered classes/functions from model_selection #5568

Closed

raghavrv mentioned this issue Mar 10, 2016

[MRG+1] MNT remove ALL_CVS and LABEL_CVS from _validation.py #6517

Merged

1 task

raghavrv mentioned this issue Apr 14, 2016

[MRG+1] ENH/MNT Rename labels --> groups #6660

Merged

9 tasks

ogrisel closed this as completed in #6660 Sep 11, 2016

ogrisel reopened this Sep 11, 2016

This was referenced Sep 12, 2016

[MRG] MNT Use isinstance instead of dtype.kind check for scalar validation. #7394

Closed

[MRG+1] TST Stronger test for _check_is_permutation #7395

Merged

amueller closed this as completed Sep 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Changes to model_selection? #5053

[RFC] Changes to model_selection? #5053

[RFC] Changes to model_selection? #5053

[RFC] Changes to model_selection? #5053

Comments