Skip to content
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

Added min_features variable to RFECV example #18091

Merged
merged 12 commits into from Aug 20, 2020

Conversation

eschibli
Copy link
Contributor

@eschibli eschibli commented Aug 4, 2020

The current example code for sklearn.feature_selection.RFECV produces confusing results if a non-default min_features_to_select parameter is added, as the example plot will be produced with an incorrect axis. Since it is not clear that the 1s in plt.plot(range(1, len(rfecv.grid_scores_) + 1), rfecv.grid_scores_) refer to the default min_features_to_select I believe it would be more informative if this were made explicit.

This is my first pull request to a public repo, so please let me know if I'm being an ass or otherwise violating etiquette.

@eschibli eschibli marked this pull request as ready for review August 4, 2020 19:36
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @eschibli !

eschibli and others added 3 commits August 6, 2020 11:55
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

One can run

./build_tools/circle/linting.sh

to see the flake errors locally.

eschibli and others added 3 commits August 6, 2020 12:08
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@eschibli
Copy link
Contributor Author

eschibli commented Aug 6, 2020

One can run

./build_tools/circle/linting.sh

to see the flake errors locally.

Thanks Thomas, that is helpful. I will do that first next time.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@eschibli
Copy link
Contributor Author

eschibli commented Aug 9, 2020

@thomasjpfan are any further actions from me required to merge the changes?

@thomasjpfan
Copy link
Member

We need to wait for another reviewer to approve.

@eschibli
Copy link
Contributor Author

eschibli commented Aug 9, 2020

Ah okay

@glemaitre glemaitre merged commit bf121f4 into scikit-learn:master Aug 20, 2020
@glemaitre
Copy link
Member

LGTM Thanks @eschibli

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants