New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] Add min_features_to_select parameter to RFECV #11293

Merged
merged 14 commits into from Aug 9, 2018

Conversation

Projects
None yet
3 participants
@brentyi
Contributor

brentyi commented Jun 15, 2018

Reference Issues/PRs

Resolves #6564
Resolves #10989
Similar to pull requests #7269 and #8812

What does this implement/fix? Explain your changes.

Adds a min_features_to_select parameter to the RFECV constructor (#6564), and uses it in the documentation to clarify the size of the grid_scores_ attribute (#10989).

Any other comments?

@brentyi brentyi changed the title from [MRG] Minimum features to to [MRG] Add min_features_to_select parameter to RFECV Jun 15, 2018

@jnothman

Otherwise LGTM

Show outdated Hide outdated sklearn/feature_selection/rfe.py Outdated
Show outdated Hide outdated sklearn/feature_selection/tests/test_rfe.py Outdated
Show outdated Hide outdated sklearn/feature_selection/tests/test_rfe.py Outdated
Show outdated Hide outdated sklearn/feature_selection/tests/test_rfe.py Outdated
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jun 19, 2018

Member
Member

jnothman commented Jun 19, 2018

@brentyi

This comment has been minimized.

Show comment
Hide comment
@brentyi

brentyi Jun 19, 2018

Contributor

@jnothman thanks for the link! that makes sense; I just moved min_features_to_select to right after step

Contributor

brentyi commented Jun 19, 2018

@jnothman thanks for the link! that makes sense; I just moved min_features_to_select to right after step

@jnothman

LGTM.
both failures look spurious.

Show outdated Hide outdated sklearn/feature_selection/tests/test_rfe.py Outdated
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jun 20, 2018

Member

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

Member

jnothman commented Jun 20, 2018

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@brentyi

This comment has been minimized.

Show comment
Hide comment
@brentyi

brentyi Jun 21, 2018

Contributor

done!

Contributor

brentyi commented Jun 21, 2018

done!

brentyi added some commits Jul 27, 2018

@jnothman jnothman changed the title from [MRG] Add min_features_to_select parameter to RFECV to [MRG+1] Add min_features_to_select parameter to RFECV Jul 29, 2018

Show outdated Hide outdated sklearn/feature_selection/rfe.py Outdated
@rth

rth approved these changes Aug 6, 2018

This LGTM except for the previous comment.

There is also a merge conflict that need to fixed though (You could merge master into this branch again).

@brentyi

This comment has been minimized.

Show comment
Hide comment
@brentyi

brentyi Aug 8, 2018

Contributor

fixed!

Contributor

brentyi commented Aug 8, 2018

fixed!

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 9, 2018

Member

Technically it's an API change, because for anyone who uses positional arguments RFECV(estimator, 1, 'warn') will have their code broken. But then I'm not sure what's out currently policy about this? Do we put the added parameter at the end (after verbose and n_jobs) or insert earlier as done here @jnothman ?

We've had an informal policy that most things are to be passed as kwargs... We couldn't enforce it easily in Python 2. I noted this under backwards compatibility in the Glossary:

We may sometimes assume that all optional parameters (other than X and y to fit and similar methods) are passed as keyword arguments only and may be positionally reordered.

But we should be a little careful about it in any case...

Member

jnothman commented Aug 9, 2018

Technically it's an API change, because for anyone who uses positional arguments RFECV(estimator, 1, 'warn') will have their code broken. But then I'm not sure what's out currently policy about this? Do we put the added parameter at the end (after verbose and n_jobs) or insert earlier as done here @jnothman ?

We've had an informal policy that most things are to be passed as kwargs... We couldn't enforce it easily in Python 2. I noted this under backwards compatibility in the Glossary:

We may sometimes assume that all optional parameters (other than X and y to fit and similar methods) are passed as keyword arguments only and may be positionally reordered.

But we should be a little careful about it in any case...

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated

brentyi and others added some commits Aug 9, 2018

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 9, 2018

Member

I'm good to merge on green.

Member

jnothman commented Aug 9, 2018

I'm good to merge on green.

@rth rth merged commit e28a577 into scikit-learn:master Aug 9, 2018

7 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 93.08%)
Details
codecov/project 93.09% (+<.01%) compared to d990f72
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rth

This comment has been minimized.

Show comment
Hide comment
@rth

rth Aug 9, 2018

Member

Thanks @brentyi !

We've had an informal policy that most things are to be passed as kwargs... We couldn't enforce it easily in Python 2. I noted this under backwards compatibility in the Glossary:

Ahh, I see. Thanks!

Member

rth commented Aug 9, 2018

Thanks @brentyi !

We've had an informal policy that most things are to be passed as kwargs... We couldn't enforce it easily in Python 2. I noted this under backwards compatibility in the Glossary:

Ahh, I see. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment