-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add one-way partial dependence and partial dependence plots #1079
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1079 +/- ##
=======================================
Coverage 99.89% 99.89%
=======================================
Files 192 192
Lines 10894 10981 +87
=======================================
+ Hits 10883 10970 +87
Misses 11 11
Continue to review full report at Codecov.
|
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.
@angela97lin looks good!
I have two change requests before merging:
- The graph is backwards! The axes need to be flipped. Noticed because the user guide example is not a function
- I found a way to get catboost working
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.
@angela97lin Looks good to me! I have a minor comment on the user guide that is not blocking.
Closes #985
scikit-learn
'spartial_dependence
does some checking to ensure that a fitted pipeline has been passed in. Unfortunately, the code used also checks for certain attributes to ensure that a classifier / regressor is passed in. There are two options:_classes
attribute toclasses_
instead to matchscikit-learn
, then set_estimator_type
for the input pipeline to something (and somehow revert it after? Tried looking into context managers but not sure if that's the right approach since we're creating a new attribute, not temporarily updating a pre-existing one).For now, I've updated this PR for option 1, since the idea of creating a new attribute and temporarily setting it feels weird, but this really limits what we can pass into
partial_dependence
, especially when it seems like it works fine (tested briefly with catboost) once we pass all of the checks. We also can't wrap the inputted pipeline with our own pipeline attribute since a fitted pipeline is required.