-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
FIX handle subsampling for sample_weight in partial_dependence #28184
Conversation
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. |
There was a problem hiding this 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
There was a problem hiding this 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.
…t-learn#28184) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
…t-learn#28184) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
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.