8000 FEA Add metadata routing for SequentialFeatureSelector by OmarManzoor · Pull Request #29260 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

FEA Add metadata routing for SequentialFeatureSelector #29260

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

Merged
merged 10 commits into from
Jul 31, 2024

Conversation

OmarManzoor
Copy link
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

Copy link
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
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.

8000

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

@@ -201,11 +202,23 @@ def fit(self, X, y=None):
Target values. This parameter may be ignored for
unsupervised learning.

**params : dict, default=None
Parameters to be passed to the cross_val_score function.
Copy link
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
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.

Co-authored-by: Adam Li <adam2392@gmail.com>
Copy link
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
Member

I'll put this one on my todo list

Copy link
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.

@adrinjalali adrinjalali merged commit 234260d into scikit-learn:main Jul 31, 2024
30 checks passed
@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
0