8000 FEA SLEP006: Metadata routing for `validation_curve` by StefanieSenger · Pull Request #29329 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

FEA SLEP006: Metadata routing for validation_curve #29329

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

Conversation

StefanieSenger
Copy link
Contributor

Reference Issues/PRs

towards #22893

What does this implement/fix? Explain your changes.

Adds metadata routing to validation_curve and the corresponding tests.
This PR is almost identical to #28975

Copy link
github-actions bot commented Jun 21, 2024

✔️ Linting Passed

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

Generated for commit: b107cfa. 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.

Thanks for the PR @StefanieSenger! Left a few comments.

Comment on lines 2347 to 2352
Parameters directly passed to the `fit` method of the estimator.

- If `enable_metadata_routing=True`:
Parameters safely routed to the `fit` method of the estimator.
See :ref:`Metadata Routing User Guide <metadata_routing>` for more
details.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. Since it's passed to more than the estimator fit. There's the scorer and CV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @adam2392, that comment also made it more clear for myself.

Co-authored-by: Adam Li <adam2392@gmail.com>
Copy link
Contributor Author
@StefanieSenger StefanieSenger left a comment

Choose a reason for hiding this comment

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

I've submitted your suggestions, @adam2392, thanks for your review.

Comment on lines 2347 to 2352
Parameters directly passed to the `fit` method of the estimator.

- If `enable_metadata_routing=True`:
Parameters safely routed to the `fit` method of the estimator.
See :ref:`Metadata Routing User Guide <metadata_routing>` for more
details.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @adam2392, that comment also made it more clear for myself.

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 now!

Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

I think this is missing a test to check for deprecation of fit_params, otherwise LGTM.

@StefanieSenger
Copy link
Contributor Author

I think this is missing a test to check for deprecation of fit_params, otherwise LGTM.

@adrinjalali
It's added to test_fit_param_deprecation in fact.

@adrinjalali adrinjalali merged commit e1cf244 into scikit-learn:main Jul 5, 2024
30 checks passed
@StefanieSenger StefanieSenger deleted the metadata_validation_curve branch July 5, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0