Skip to content

FEA Add metadata routing for SequentialFeatureSelector#29260

Merged
adrinjalali merged 10 commits into
scikit-learn:mainfrom
OmarManzoor:sfs_routing
Jul 31, 2024
Merged

FEA Add metadata routing for SequentialFeatureSelector#29260
adrinjalali merged 10 commits into
scikit-learn:mainfrom
OmarManzoor:sfs_routing

Conversation

@OmarManzoor
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Towards: #22893

What does this implement/fix? Explain your changes.

  • Adds metadata routing for SequentialFeatureSelector

Any other comments?

CC: @adrinjalali @glemaitre

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 14, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: e8ff531. Link to the linter CI: here

Copy link
Copy Markdown
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

I left a few comments. Let me know if I misunderstood something tho.

unsupervised learning.

**params : dict, default=None
Parameters to be passed to the cross_val_score function.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Parameters to be passed to the cross_val_score function.
Parameters to be passed to the scoring function evaluated through cross validation
via the `cross_val_score` function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Within the cross_val_score function I think we route to more than just the scoring function. We also route to cv and estimator fit methods.

Comment thread sklearn/feature_selection/_sequential.py
Comment thread sklearn/tests/test_metaestimators_metadata_routing.py
Comment thread sklearn/feature_selection/_sequential.py Outdated
Co-authored-by: Adam Li <adam2392@gmail.com>
Copy link
Copy Markdown
Member

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

LGTM if the CI passes!

@glemaitre glemaitre self-requested a review July 9, 2024 09:20
@glemaitre
Copy link
Copy Markdown
Member

I'll put this one on my todo list

Comment thread sklearn/feature_selection/_sequential.py Outdated
Comment thread sklearn/feature_selection/tests/test_sequential.py Outdated
Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

This is a partial review.

Comment thread sklearn/feature_selection/_sequential.py Outdated
Comment thread sklearn/feature_selection/tests/test_sequential.py Outdated
@adrinjalali adrinjalali merged commit 234260d into scikit-learn:main Jul 31, 2024
@OmarManzoor OmarManzoor deleted the sfs_routing branch July 31, 2024 13:59
MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024
…29260)

Co-authored-by: Adam Li <adam2392@gmail.com>
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants