Skip to content

FEA SLEP006: Metadata routing for validation_curve#29329

Merged
adrinjalali merged 8 commits into
scikit-learn:mainfrom
StefanieSenger:metadata_validation_curve
Jul 5, 2024
Merged

FEA SLEP006: Metadata routing for validation_curve#29329
adrinjalali merged 8 commits into
scikit-learn:mainfrom
StefanieSenger:metadata_validation_curve

Conversation

@StefanieSenger

Copy link
Copy Markdown
Member

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

@github-actions

github-actions Bot commented Jun 21, 2024

Copy link
Copy Markdown

✔️ Linting Passed

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

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

@adam2392 adam2392 left a comment

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.

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

Comment thread sklearn/model_selection/_validation.py Outdated
Comment thread sklearn/model_selection/_validation.py Outdated
Comment thread sklearn/model_selection/_validation.py Outdated
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
Copy Markdown
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
Copy Markdown
Member 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>

@StefanieSenger StefanieSenger left a comment

Copy link
Copy Markdown
Member Author

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 thread sklearn/model_selection/_validation.py Outdated
Comment thread sklearn/model_selection/_validation.py Outdated
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
Copy Markdown
Member 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.

@adam2392 adam2392 left a comment

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.

LGTM now!

@adrinjalali adrinjalali left a comment

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.

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

@StefanieSenger

Copy link
Copy Markdown
Member 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
@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