ENH Allow SelectFromModel's max_features to accept callables#22356
Merged
adrinjalali merged 43 commits intoscikit-learn:mainfrom Mar 22, 2022
Merged
ENH Allow SelectFromModel's max_features to accept callables#22356adrinjalali merged 43 commits intoscikit-learn:mainfrom
SelectFromModel's max_features to accept callables#22356adrinjalali merged 43 commits intoscikit-learn:mainfrom
Conversation
SelectFromModel's max_features to accept floats and callables
thomasjpfan
reviewed
Feb 2, 2022
Member
thomasjpfan
left a comment
There was a problem hiding this comment.
Having both floats and callables in a single PR may stall it. I think it's best to pick one for the first PR.
Callable is the more interesting one since it will be the first time we will support a callable for a *_features parameter.
thomasjpfan
reviewed
Feb 2, 2022
Member
thomasjpfan
left a comment
There was a problem hiding this comment.
Thanks for the update! This looks good.
For new code, I prefer using single backticks. (We changed the setting ~2.5 years ago to make single backticks == double backticks)
SelectFromModel's max_features to accept floats and callablesSelectFromModel's max_features to accept callables
Contributor
Author
|
Updated the code to incorporate your suggestions! I noticed the different styles of back-ticks and was curious about that -- thanks for clearing it up! Right now there's not really any style standardization regarding single vs double back-ticks and quotes right? |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…4/scikit-learn into sfm_max_features_upgrade
Contributor
Author
|
@adrinjalali @glemaitre Would either of you be interested in reviewing this PR? |
Member
|
The CI failures are related to the changes in this PR. |
adrinjalali
reviewed
Mar 17, 2022
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
adrinjalali
approved these changes
Mar 18, 2022
Member
|
CI failure is related to our doc changes. I'm not sure how to fix them (has to to with indentations and new lines). You can try and see if adding new lines or changing indentation would fix it: |
Contributor
Author
|
Should be good to go now! |
glemaitre
pushed a commit
to glemaitre/scikit-learn
that referenced
this pull request
Apr 6, 2022
…kit-learn#22356) * Initial implementation * Improved error handling and stability * Added unit tests * Updated test to use `max_features_` instead of `max_features` * Added documentation for new private attribute `max_features_` * Improved error handling for callables * Updated whats_new * Removed incorrect term reference to `max_features` * Removed float case and improved testing * Updated test names to more clearly reflect intention * Added a sample callable in `max_features` description * Improved documentation and streamlined error handling * Updated example to include demonstrate using a callable for max_features * Separated out callable demo into separate example * Removed demo from `max_features` docs (now in example) * Updated changelog * Apply suggestions from code review Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * Trimmed unneeded comments * Updated tests to reflect new error handling * Removed new line at end of docstring * Updated docstring * Fixed example syntax error * Fixed example syntax * Apply suggestions from code review Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * Reverted irrelevant changes * Update sklearn/feature_selection/_from_model.py Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * Fixed error message * Improved test coverage * Minor doc improvement -- added a list for `max_features` type * Update sklearn/feature_selection/_from_model.py Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> * Improved input validation and added test for array-like * Updated doc to use no longer use lambda function * Fixed docstring list * Added missing whitespace for list format in docstring Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
Fixes #21117
What does this implement/fix? Explain your changes.
This PR improves
SelectFromModel'smax_featuresby allowing it to:To accomplish this, I introduced a new private attribute
max_features_which is calculated infitbased on what type was received formax_features. The calculations for integral, and callable values, e.g.:max_features_=max_featuresmax_features_=round(max_features*X.shape[1])max_features_=max_features(X)I then augmented existing unit tests to account for these new cases
Any other comments?