Skip to content

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

Merged
amueller merged 1 commit intoscikit-learn:masterfrom
raghavrv:model_selection_enhancements1
Sep 12, 2016
Merged

[MRG+1] MNT remove ALL_CVS and LABEL_CVS from _validation.py#6517
amueller merged 1 commit intoscikit-learn:masterfrom
raghavrv:model_selection_enhancements1

Conversation

@raghavrv
Copy link
Copy Markdown
Member

@raghavrv raghavrv commented Mar 10, 2016 •
edited
Loading

A minor part of #5053

  • Move the list of CV iterators - *CV to end of _split and use it for testing

@jnothman @amueller @MechCoder @vene

@raghavrv raghavrv force-pushed the model_selection_enhancements1 branch from 8f7bea9 to a2c3922 Compare April 14, 2016 10:00
@raghavrv raghavrv changed the title [WIP] ENH Various enhancements to the model_selection module [WIP] ENH Various minor enhancements to the model_selection module Apr 19, 2016
@raghavrv raghavrv changed the title [WIP] ENH Various minor enhancements to the model_selection module [MRG] MNT move ALL_CVS and LABEL_CVS to the end of _split and use it for testing Aug 19, 2016
@raghavrv raghavrv force-pushed the model_selection_enhancements1 branch 2 times, most recently from 3b812a8 to 360e86d Compare August 19, 2016 16:04
@raghavrv
Copy link
Copy Markdown
Member Author

raghavrv commented Aug 19, 2016

@jnothman @amueller This is a minor one. Review and Merge?

jnothman
jnothman reviewed Aug 20, 2016
View reviewed changes
sklearn/model_selection/_split.py Outdated
return '%s(%s)' % (class_name, _pprint(params, offset=len(class_name)))


ALL_CVS = {'KFold': KFold,
Copy link
Copy Markdown
Member

@jnothman jnothman Aug 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these being made available to the public?

Copy link
Copy Markdown
Member

@jnothman jnothman Aug 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they're only for testing, they probably belong in tests, no?

Copy link
Copy Markdown
Member Author

@raghavrv raghavrv Aug 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to make it public like SCORERS I think... Do you feel it would not add much value being public?

Copy link
Copy Markdown
Member

@jnothman jnothman Aug 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCORERS was public because get_scorer didn't exist, and because it's a register of (non-importable) strings, not a collection of classes that can be imported. I'm a little ambivalent, but this would be novel in scikit-learn. __all__ is close to the same...?

@raghavrv
Copy link
Copy Markdown
Member Author

raghavrv commented Aug 21, 2016

Ok. I'll move it from validation to the tests for now...

@amueller
Copy link
Copy Markdown
Member

amueller commented Aug 26, 2016

I'd rather not add public constants unless necessary.

@raghavrv raghavrv force-pushed the model_selection_enhancements1 branch from 9b4329d to a25d7c8 Compare August 29, 2016 13:16
@raghavrv
Copy link
Copy Markdown
Member Author

raghavrv commented Sep 8, 2016

selfnote: Simply remove the existing constants from the file and don't touch the tests...

@raghavrv raghavrv force-pushed the model_selection_enhancements1 branch from a25d7c8 to 8dd1a71 Compare September 12, 2016 09:24
@raghavrv
Copy link
Copy Markdown
Member Author

raghavrv commented Sep 12, 2016

@amueller @jnothman This removes the constants from _validation.py. We can add them to tests when we need it.

Review and merge please?

@raghavrv raghavrv closed this Sep 12, 2016
@raghavrv raghavrv force-pushed the model_selection_enhancements1 branch from 8dd1a71 to b966b44 Compare September 12, 2016 09:26
@raghavrv raghavrv reopened this Sep 12, 2016
@raghavrv raghavrv changed the title [MRG] MNT move ALL_CVS and LABEL_CVS to the end of _split and use it for testing [MRG] MNT remove ALL_CVS and LABEL_CVS from _validation.py Sep 12, 2016
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Sep 12, 2016

LGTM

@jnothman jnothman changed the title [MRG] MNT remove ALL_CVS and LABEL_CVS from _validation.py [MRG+1] MNT remove ALL_CVS and LABEL_CVS from _validation.py Sep 12, 2016
@jnothman jnothman added this to the 0.18 milestone Sep 12, 2016
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Sep 12, 2016

@ogrisel, can we merge this before accidentally making commitments to public namespace?

@amueller amueller merged commit c3b2115 into scikit-learn:master Sep 12, 2016
@raghavrv raghavrv deleted the model_selection_enhancements1 branch September 12, 2016 14:32
@raghavrv raghavrv mentioned this pull request Sep 13, 2016
Closed
rsmith54 pushed a commit to rsmith54/scikit-learn that referenced this pull request Sep 14, 2016
@raghavrv
MNT Remove unused constants from public API(?) (scikit-learn#6517)
ce8d7c7
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016
@raghavrv @TomDLT
MNT Remove unused constants from public API(?) (scikit-learn#6517)
a254694
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@raghavrv @Sundrique
MNT Remove unused constants from public API(?) (scikit-learn#6517)
7f1f672
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@raghavrv @paulha
MNT Remove unused constants from public API(?) (scikit-learn#6517)
c7703b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Blocker

Projects

None yet

Milestone

0.18

Development

Successfully merging this pull request may close these issues.

3 participants

@raghavrv @amueller @jnothman