-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add Recursive Feature Elimination Selector component #3934
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3934 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 346 347 +1
Lines 36719 36755 +36
=======================================
+ Hits 36589 36625 +36
Misses 130 130
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
7fe2095
to
7667f33
Compare
hyperparameter_ranges = { | ||
"perc": Integer(0, 100), | ||
} | ||
"""{ | ||
"percent_features": Real(0.01, 1), | ||
"threshold": ["mean", "median"], | ||
}""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd want min_features_to_select
and step
here but not positive on how this section is mean to be set up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good catch. I overlooked that, but will take a closer look and update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should set up step
between 0 and 1 so it selects a eliminates a percentage amount (so we can generalize more easily than an integer) but I don't think we need to tune min_features_to_select
since RFECV is supposed to find the optimal amount of features. I think a default min_features_to_select=1
is fine unless there's a reason we want RFE to keep more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to have step between 0.05 and 0.25 and changed the default for min_features_to_select
to 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to fix up the hyperparameter ranges but LGTM otherwise!
"""Selects relevant features using recursive feature elimination.""" | ||
|
||
hyperparameter_ranges = { | ||
"perc": Integer(0, 100), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perc
isn't used anywhere right? This should be updated with step
and other parameters we want to tune.
Defaults to None. | ||
n_estimators (float): The number of trees in the forest. Defaults to 100. | ||
max_depth (int): Maximum tree depth for base learners. Defaults to 6. | ||
If both percent_features and number_features are specified, take the greater number of features. Defaults to None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the fix 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add Recursive Feature Elimination Selector component
Adds a new feature selector component that uses scikit-learn's RFECV to perform the selection.