Non Numeric Bug with Partial Dependence#1748
Conversation
chukarsten
force-pushed
the
1509-part_dep_non_numeric_bug
branch
2 times, most recently
from
927864f to
f8cf441
Compare
Codecov Report
@@ Coverage Diff @@
## main #1748 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 247 247
Lines 19571 19598 +27
=========================================
+ Hits 19562 19589 +27
Misses 9 9
Continue to review full report at Codecov.
|
chukarsten
force-pushed
the
1509-part_dep_non_numeric_bug
branch
2 times, most recently
from
92b35f9 to
05ee3bd
Compare
chukarsten
requested review from
ParthivNaresh,
angela97lin,
bchen1116,
christopherbunn,
dsherry,
freddyaboulton and
jeremyliweishih
and removed request for
dsherry and
freddyaboulton
ParthivNaresh
left a comment
ParthivNaresh
left a comment
There was a problem hiding this comment.
Everything looks good, just wanted to clarify the docstring wording
evalml/model_understanding/graphs.py
Outdated
| If features is a tuple of int/strings, it must contain valid column integers/names in X. | ||
| grid_resolution (int): Number of samples of feature(s) for partial dependence plot | ||
| grid_resolution (int): Number of samples of feature(s) for partial dependence plot. If this value | ||
| does not exceed the maximum number of categories present in categorical data within X, it will be |
There was a problem hiding this comment.
If this number does not exceed or if this number does exceed?
There was a problem hiding this comment.
I think the wording could be clarified a bit. If grid_resolution is less than the maximum number of..etc`
angela97lin
left a comment
angela97lin
left a comment
There was a problem hiding this comment.
Looking good 😁
I think we can clean up the impl and test a bit but thank you for fixing this!!
| partial_dependence(pipeline, X, features=(0, 'b')) | ||
|
|
||
|
|
||
| def test_partial_dependence_more_categories_than_grid_resolution(logistic_regression_binary_pipeline_class): |
There was a problem hiding this comment.
Just a suggestion but maybe we could somehow make it more clear that the number of categories is greater than the grid resolution, whether that's by setting grid_resolution specifically to a number and then asserting that there are currency has more than that number of categories, which is why we trigger this new code? Or alternatively we could have a tiny dataset like the previous test and set grid_resolution to something even tinier?
…ing error between different versions.
chukarsten
force-pushed
the
1509-part_dep_non_numeric_bug
branch
from
05ee3bd to
297d622
Compare
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
@chukarsten Looks good to me! Thanks for the fix 😄
angela97lin
left a comment
angela97lin
left a comment
There was a problem hiding this comment.
Looks great! Thank you for updating the impl and tests 😁
4 participants
fixes #1509