[MRG] Enable percentage for n_features_to_select in RFE(Fix PR #16228) #17090
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lschwetlick
changed the title
|
Hi @lschwetlick, do you mind fixing the conflicts? Your PR will close a number of stalled PR, I think we should push it to be reviewed. Thanks! |
|
Yes, I hope it's fixed it now! |
rth
reviewed
Jun 9, 2020
There was a problem hiding this comment.
Minor comments otherwise LGTM. Thanks @lschwetlick !
Please add an entry to the change log at doc/whats_new/v0.24.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
|
Alright, I integrated your suggestions @rth . Looks good? |
thomasjpfan
reviewed
Jun 17, 2020
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
|
Hopefully @thomasjpfan is there to catch all the things I miss :) |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…learn into n_features_to_select
|
@thomasjpfan did I get everything? |
thomasjpfan
reviewed
Jun 17, 2020
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>
thomasjpfan
reviewed
Jun 18, 2020
| def test_rfe_negative_n_features(): | ||
| clf = SVC(kernel="linear") | ||
| with pytest.raises(ValueError, match=r"n_features_to_select must be *"): | ||
| RFE(estimator=clf, n_features_to_select=-1, step=0.1).fit(...) |
There was a problem hiding this comment.
My fit(...) suggestion means that ... should be replaced with something meaningful. In this case:
iris = load_iris()
rfe = RFE(estimator=clf, n_features_to_select=-1, step=0.1)
with pytest.raises(ValueError, match=r"n_features_to_select must be *"):
rfe.fit(iris.data, iris.target)There was a problem hiding this comment.
Ah, yes of course, sorry, I should have seen that!
thomasjpfan
reviewed
Jun 18, 2020
|
After taking a look at the trees code, I made the error message more explicit and added the case you asked for! |
thomasjpfan
approved these changes
Jun 27, 2020
glemaitre
moved this from TO REVIEW
to REVIEWED AND WAITING FOR CHANGES
in Guillaume's pet
glemaitre
pushed a commit
to glemaitre/scikit-learn
that referenced
this pull request
Jul 17, 2020
…earn#16228) (scikit-learn#17090) * added code from PR scikit-learn#14627 * fixed error handling None as n_features_to_select * added test for error message and percentage passing * linting * more linting * even more linting * sklearn/feature_selection/_rfe.py make int casting simpler Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com> * exchange redundant test case with small float * add what's new * removed useless import * lint * Fix whats_new/v0.24.rst Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * move whats new section * disambiguate choice between 1 feature and 100% of features Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * move checking for negatives to _fit * update error to include nones Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * markdown style in what's new Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * Update sklearn/feature_selection/tests/test_rfe.py Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * forgot to delete line * fix test for negative n_features error * BUG Fixes issues * added case for float >1 and more detailed error messages * lint * more lint * more lint * CLN Minor adjustments * BLD Force build on ci Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
jayzed82
pushed a commit
to jayzed82/scikit-learn
that referenced
this pull request
Oct 22, 2020
…earn#16228) (scikit-learn#17090) * added code from PR scikit-learn#14627 * fixed error handling None as n_features_to_select * added test for error message and percentage passing * linting * more linting * even more linting * sklearn/feature_selection/_rfe.py make int casting simpler Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com> * exchange redundant test case with small float * add what's new * removed useless import * lint * Fix whats_new/v0.24.rst Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * move whats new section * disambiguate choice between 1 feature and 100% of features Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * move checking for negatives to _fit * update error to include nones Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * markdown style in what's new Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * Update sklearn/feature_selection/tests/test_rfe.py Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * forgot to delete line * fix test for negative n_features error * BUG Fixes issues * added case for float >1 and more detailed error messages * lint * more lint * more lint * CLN Minor adjustments * BLD Force build on ci Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
This brings PR #16228 up to date, adds tests and fixes some issues with it.
That PR was meant to Fix #14567, close #14627 and close #15529 by adding the option to select n features by percentage instead of by absolute number.
The changes PR #16228 made to
isotronic.pyhave since been added already and are not included in this PR. Here I fix some bugs from PR #16228 and add tests for the new functionality.Ping @noatamir