[MRG] Enable percentage for n_features_to_select in RFE(Fix PR #16228) by lschwetlick · Pull Request #17090 · scikit-learn/scikit-learn

lschwetlick · 2020-04-30T10:23:20Z

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.py have 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

cmarmo · 2020-06-09T07:33:47Z

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!

lschwetlick · 2020-06-09T09:02:33Z

Yes, I hope it's fixed it now!

rth

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 <[email protected]>

lschwetlick · 2020-06-17T17:29:53Z

Alright, I integrated your suggestions @rth . Looks good?

rth

LGTM, thanks @lschwetlick !

thomasjpfan

Thank you for the PR @lschwetlick !

Co-authored-by: Thomas J. Fan <[email protected]>

rth · 2020-06-17T17:58:31Z

Hopefully @thomasjpfan is there to catch all the things I miss :)

Co-authored-by: Thomas J. Fan <[email protected]>

…learn into n_features_to_select

lschwetlick · 2020-06-17T18:08:57Z

@thomasjpfan did I get everything?

Co-authored-by: Thomas J. Fan <[email protected]>

thomasjpfan · 2020-06-18T14:30:16Z

+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(...)


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)

Ah, yes of course, sorry, I should have seen that!

thomasjpfan

I made a few small updates to this PR. I have one more thought regarding error when n_features_to_select is a float and > 1.0.

lschwetlick · 2020-06-25T07:16:09Z

After taking a look at the trees code, I made the error message more explicit and added the case you asked for!

thomasjpfan

Thank you @lschwetlick !

LGTM

…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 <[email protected]> * 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 <[email protected]> * move whats new section * disambiguate choice between 1 feature and 100% of features Co-authored-by: Thomas J. Fan <[email protected]> * move checking for negatives to _fit * update error to include nones Co-authored-by: Thomas J. Fan <[email protected]> * markdown style in what's new Co-authored-by: Thomas J. Fan <[email protected]> * Update sklearn/feature_selection/tests/test_rfe.py Co-authored-by: Thomas J. Fan <[email protected]> * 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 <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

lschwetlick added 3 commits April 30, 2020 10:52

added code from PR scikit-learn#14627

7822e39

fixed error handling None as n_features_to_select

fdcce14

added test for error message and percentage passing

f4ba4b6

github-actions Bot added the module:feature_selection label Apr 30, 2020

lschwetlick added 3 commits April 30, 2020 12:26

linting

9d343d4

more linting

14d8c39

even more linting

ce8ed9e

lschwetlick changed the title ~~[WIP] Enable percentage for n_features_to_select in RFE(Fix PR #16228)~~ [MRG] Enable percentage for n_features_to_select in RFE(Fix PR #16228) Apr 30, 2020

cmarmo added the Waiting for Reviewer label May 4, 2020

Merge branch 'master' into n_features_to_select

5fe6f8a

cmarmo requested a review from rth June 9, 2020 09:06

rth reviewed Jun 9, 2020

View reviewed changes

Comment thread sklearn/feature_selection/tests/test_rfe.py Outdated

Comment thread sklearn/feature_selection/_rfe.py Outdated

lschwetlick and others added 4 commits June 17, 2020 19:09

sklearn/feature_selection/_rfe.py make int casting simpler

8ef3212

Co-authored-by: Roman Yurchak <[email protected]>

exchange redundant test case with small float

ea29314

add what's new

3b23cb9

removed useless import

7487d94

lint

9b8aaaf

rth approved these changes Jun 17, 2020

View reviewed changes

thomasjpfan reviewed Jun 17, 2020

View reviewed changes

Comment thread sklearn/feature_selection/_rfe.py Outdated

Comment thread doc/whats_new/v0.24.rst Outdated

Comment thread doc/whats_new/v0.24.rst Outdated

Comment thread sklearn/feature_selection/_rfe.py Outdated

Fix whats_new/v0.24.rst

b01cc4d

Co-authored-by: Thomas J. Fan <[email protected]>

lschwetlick and others added 4 commits June 17, 2020 19:59

move whats new section

c728628

disambiguate choice between 1 feature and 100% of features

d4e0fad

Co-authored-by: Thomas J. Fan <[email protected]>

Merge branch 'n_features_to_select' of github.com:lschwetlick/scikit-…

21df48c

…learn into n_features_to_select

move checking for negatives to _fit

9780746

thomasjpfan reviewed Jun 17, 2020

View reviewed changes

Comment thread sklearn/feature_selection/_rfe.py Outdated

Comment thread sklearn/feature_selection/_rfe.py Outdated

Comment thread sklearn/feature_selection/tests/test_rfe.py Outdated

Comment thread doc/whats_new/v0.24.rst

lschwetlick and others added 4 commits June 18, 2020 00:12

update error to include nones

c46be2d

Co-authored-by: Thomas J. Fan <[email protected]>

markdown style in what's new

37608fe

Co-authored-by: Thomas J. Fan <[email protected]>

Update sklearn/feature_selection/tests/test_rfe.py

09549e3

Co-authored-by: Thomas J. Fan <[email protected]>

forgot to delete line

a024c5f

cmarmo removed the Waiting for Reviewer label Jun 18, 2020

thomasjpfan reviewed Jun 18, 2020

View reviewed changes

lschwetlick and others added 2 commits June 18, 2020 17:29

fix test for negative n_features error

fe1cff0

BUG Fixes issues

074801c

thomasjpfan reviewed Jun 18, 2020

View reviewed changes

Comment thread sklearn/feature_selection/_rfe.py Outdated

Comment thread sklearn/feature_selection/_rfe.py Outdated

Comment thread sklearn/feature_selection/_rfe.py

added case for float >1 and more detailed error messages

6b0002f

lschwetlick and others added 6 commits June 25, 2020 09:17

lint

1d9fa90

more lint

942d70a

more lint

9645401

CLN Minor adjustments

a44be51

Merge remote-tracking branch 'upstream/master' into pr/17090

2627817

BLD Force build on ci

4b9dc69

thomasjpfan approved these changes Jun 27, 2020

View reviewed changes

thomasjpfan merged commit 911137f into scikit-learn:master Jun 27, 2020

cmarmo mentioned this pull request Jun 29, 2020

[WIP] Modified n_features_to_select to accept float as a percentage of features to select #16228

Closed

Conversation

lschwetlick commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmarmo commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lschwetlick commented Jun 9, 2020

Uh oh!

rth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lschwetlick commented Jun 17, 2020

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rth commented Jun 17, 2020

Uh oh!

lschwetlick commented Jun 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasjpfan Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

lschwetlick Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lschwetlick commented Jun 25, 2020

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lschwetlick commented Apr 30, 2020 •

edited

Loading

cmarmo commented Jun 9, 2020 •

edited

Loading

rth left a comment •

edited

Loading