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+2] Fix edge case of tied CV scores in RFECV #9222

Merged
merged 5 commits into from May 24, 2018

Conversation

Projects
None yet
4 participants
@nickypie
Contributor

nickypie commented Jun 25, 2017

In the feature_selection module, RFECV selects the model with the
highest cross-validation score. In the event of CV score ties, one
expects RFECV to return the best model with the fewest features.
This fix addresses such an edge case where two or more models have
identical cross-validation scores.

Fix edge case of tied CV scores in RFECV
In the feature_selection module, RFECV selects the model with the
highest cross-validation score. In the event of CV score ties, one
expects RFECV to return the best model with the fewest features.
This fix addresses such an edge case where two or more models have
identical cross-validation scores.
@jnothman

This LGTM. Please add an entry to what's new.

@jnothman jnothman changed the title from [MRG] Fix edge case of tied CV scores in RFECV to [MRG+1] Fix edge case of tied CV scores in RFECV Jun 25, 2017

@jnothman jnothman added the Bug label Jun 26, 2017

@amueller

This comment has been minimized.

Member

amueller commented May 23, 2018

Can you please change the whatsnew entry to the new organization in the version specific whatsnew?

@amueller

This comment has been minimized.

Member

amueller commented May 23, 2018

LGTM

@amueller amueller changed the title from [MRG+1] Fix edge case of tied CV scores in RFECV to [MRG+2] Fix edge case of tied CV scores in RFECV May 23, 2018

@amueller

This comment has been minimized.

Member

amueller commented May 23, 2018

@rth you lost the whatsnew ;)

@rth

This comment has been minimized.

Member

rth commented May 23, 2018

you lost the whatsnew ;)

@amueller I know, was planning to fix it in a separate commit. This should be fixed now. I haven't realized you approved this only 2 hours ago..

@amueller

This comment has been minimized.

Member

amueller commented May 24, 2018

@rth I'm only doing stuff since Monday ;)

@@ -471,6 +471,11 @@ Preprocessing
``inverse_transform`` on unseen labels. :issue:`9816` by :user:`Charlie Newey
<newey01c>`.
Feature selection
- Fixed computation of `n_features_to_compute` for edge case with tied

This comment has been minimized.

@amueller

amueller May 24, 2018

Member

single backticks do nothing. double backticks!

This comment has been minimized.

@rth

rth May 24, 2018

Member

Thanks, done.

@amueller amueller merged commit 1755b89 into scikit-learn:master May 24, 2018

8 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 95.12%)
Details
codecov/project 95.12% (+<.01%) compared to 84f5ee6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment