[go: up one dir, main page]

Skip to content
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

FIX handle subsampling for sample_weight in partial_dependence #28184

Merged

Conversation

mayer79
Copy link
Contributor
@mayer79 mayer79 commented Jan 19, 2024

Reference Issues/PRs

Fixes #28183.

What does this implement/fix? Explain your changes.

If permutation importance is applied with row subsampling, and sample_weight is passed, this PR makes sure that also sample_weight is subsampled.

Copy link
github-actions bot commented Jan 19, 2024

✔️ Linting Passed

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

Generated for commit: 2b68787. Link to the linter CI: here

@glemaitre
Copy link
Member

Thanks @Meyer79. We will target to have this merge for 1.4.1. I'll put it in my list for review. It should be quite straightforward.

@glemaitre glemaitre added this to the 1.4.1 milestone Jan 26, 2024
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.

This needs a test and a changelog entry under 1.4.1

@glemaitre glemaitre changed the title Fix subsampling for sample_weight FIX handle subsampling for sample_weight in partial_dependence Feb 1, 2024
@glemaitre glemaitre self-requested a review February 1, 2024 16:52
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.

I push a "light" non-regression test. We could make the tests statistically stronger to be sure that we properly index sample_weight.

For 1.4.1, I think this is enough here but we can revisit later.

@adrinjalali adrinjalali enabled auto-merge (squash) February 1, 2024 18:27
@adrinjalali adrinjalali merged commit 548fc6f into scikit-learn:main Feb 1, 2024
27 checks passed
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 10, 2024
…t-learn#28184)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 13, 2024
…t-learn#28184)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
glemaitre added a commit that referenced this pull request Feb 13, 2024
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
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.

inspection.permutation_importance: max_samples does not work with sample_weight
3 participants