10000 Make FrozenEstimator explicitly accept and ignore sample_weight by ogrisel · Pull Request #30874 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Make FrozenEstimator explicitly accept and ignore sample_weight #30874

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 24, 2025
Merged

Make FrozenEstimator explicitly accept and ignore sample_weight #30874

merged 8 commits into from
Feb 24, 2025

Conversation

ogrisel
Copy link
Member
@ogrisel ogrisel commented Feb 21, 2025

This is in particular necessary to avoid spurious warnings as triggered in https://github.com/scikit-learn/scikit-learn/pull/30873/files#r1965699574.

@github-actions GitHub Actions
Copy link
github-actions bot commented Feb 21, 2025 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 24cfcd4. Link to the linter CI: here

@ogrisel ogrisel mentioned this pull request Feb 21, 2025
Merged
@ogrisel ogrisel added the Quick Review For PRs that are quick to review label Feb 24, 2025
OmarManzoor
OmarManzoor reviewed Feb 24, 2025
View reviewed changes
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.

Thank you for the PR @ogrisel

@ogrisel @OmarManzoor
Apply suggestions from code review
47e8c96 
Co-authored-by: Omar Salman <omar.salman2007@gmail.com>
@ogrisel
Copy link
Member Author
ogrisel commented Feb 24, 2025

Thanks for the review @OmarManzoor. I addressed your feedback.

OmarManzoor
OmarManzoor approved these changes Feb 24, 2025
View reviewed changes
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. Thank you @ogrisel

@OmarManzoor OmarManzoor enabled auto-merge (squash) February 24, 2025 15:21
@OmarManzoor OmarManzoor merged commit 0b5cd27 into scikit-learn:main Feb 24, 2025
31 checks passed
@adrinjalali
Copy link
Member

A few points on this (from our call):

  • FrozenEstimator is not special here, checking if fit consumes sample_weight is always broken for all meta-estimators which do not consume sample_weight, and that's okay.
  • CalibrationClassifierCV has a warning which is raised in the case of metadata routing not being enabled. That warning message can be improved in cases where the sub-estimator does not accept sample_weight, but has a **kwargs in its signature, which means it might be a meta-estimator.

scikit-learn/sklearn/calibration.py

Line 383 in ada9947

warnings.warn(

So this PR is to be reverted, and the warning to be improved.

OmarManzoor added a commit that referenced this pull request Feb 25, 2025
@OmarManzoor
Revert "Make FrozenEstimator explicitly accept and ignore sample_weig…
3de4409 
…ht (#30874)"

This reverts commit 0b5cd27.
@OmarManzoor OmarManzoor mentioned this pull request Feb 25, 2025
Merged
@ogrisel ogrisel deleted the enh-frozen-estimator-explicitly-ignore-sample_weight branch February 28, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarManzoor OmarManzoor OmarManzoor approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ogrisel @adrinjalali @thomasjpfan @OmarManzoor
0