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

ENH Support sample weights in PartialDependenceDisplay.from_estimator #26644

Merged
merged 5 commits into from Jun 22, 2023

Conversation

vitaliset
Copy link
Contributor

This PR fixes #24872.
Built on top of #25209.

As partial dependence of a model at a point is defined as an expectation, as #24872 points out, it should respect sample_weight if someone wishes to use it (for instance, when you know your X does not follow the distribution you are interested in).

The prior PR (#25209) incorporated this logic into the partial_dependence_plot function. This current PR completes the integration by incorporating it into PartialDependenceDisplay.from_estimator.

@github-actions
Copy link

github-actions bot commented Jun 21, 2023

✔️ Linting Passed

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

Generated for commit: 956002d

@vitaliset
Copy link
Contributor Author

Kindly pinging @glemaitre, @lorentzenchr, @jeremiedbb here as you might be interested and already familiar with the previous PR.
Maybe we can add a 1.3 milestone to attract reviewers. This PR should be easy to review and easy for me to edit. ;)

I wasn't creative in the test, as it was unclear to me what we want to evaluate. Therefore, any suggestions are welcome!

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.

It looks good to me.
@jeremiedbb I think we need this one not to have a discrepancy between partial_dependence and the display in 1.3.

@jeremiedbb jeremiedbb added this to the 1.3 milestone Jun 21, 2023
@jeremiedbb jeremiedbb added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jun 21, 2023
@jeremiedbb
Copy link
Member

I'm okay with that

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@vitaliset
Copy link
Contributor Author

I just updated the branch with main. CI is green. I'm ok with it being merged as is. :)

@jeremiedbb
Copy link
Member

Thanks @vitaliset

@jeremiedbb jeremiedbb merged commit 5e8d8cb into scikit-learn:main Jun 22, 2023
28 checks passed
@vitaliset vitaliset deleted the pdp_plot_sw branch June 22, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:inspection To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

partial_dependence should respect sample weights
4 participants