Skip to content

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Jul 25, 2023

This adds metadata routing to cross_val* functions.

Note that this PR does NOT route metadata for predict method of the estimator if any.

Closes #13432
Fixes #4632
Fixes #7646
Fixes #20349

@github-actions
Copy link

github-actions bot commented Jul 25, 2023

✔️ Linting Passed

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

Generated for commit: 19c1193. Link to the linter CI: here

@adrinjalali adrinjalali marked this pull request as ready for review August 1, 2023 13:09
@adrinjalali
Copy link
Member Author

@glemaitre @OmarManzoor @thomasjpfan this is ready for review.

@glemaitre glemaitre self-requested a review August 2, 2023 14:13
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.

Just a couple of nitpicks. Otherwise, I am fine with the design.

In the future we could improve:

  • the code sharing between cross-valdiate and cross_val_predict
  • how _MultiMetricScorer is passed around. It would make sense to create such instance when checking and passing it around and to create a to_dict to convert a scorer back to what is needed later on.

adrinjalali and others added 8 commits August 3, 2023 13:36
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@adrinjalali
Copy link
Member Author

Created #27001 and #27000 to address the comments separately.

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

Generally looks good. Added a few comments.

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

A few more comments.

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @adrinjalali

@glemaitre glemaitre merged commit 6fa514a into scikit-learn:main Aug 9, 2023
@glemaitre
Copy link
Member

LGTM. Thanks @adrinjalali

@lorentzenchr
Copy link
Member

I guess we can promote this to a major feature already.

@adrinjalali
Copy link
Member Author

Soon I'll update the changelog to create a separate section for SLEP006 @lorentzenchr

TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants