-
Notifications
You must be signed in to change notification settings - Fork 533
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
Move SHAP explainers out of experimental #3596
Conversation
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.
Looks great. I mostly have just small questions. Also, let's move this sucker out of experimental!
python/cuml/test/conftest.py
Outdated
return create_synthetic_dataset(generator=skl_make_clas, | ||
n_samples=101, | ||
n_features=11, | ||
test_size=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.
test_size=1 seems like it may hide some bugs potentially? I know you're trying to save on runtime but could we get to 2 or 3?
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.
@dantegd ping on this one? Maybe I'm just misunderstanding
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.
It was missing, just added a bigger test size to the exact tests of both perm and kernel (had to update the precalculated values because the seed changed when changing the sizes here)
@@ -69,9 +53,13 @@ def experimental_test_and_log(cu_shap_values, | |||
print("golden_result_values") |
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.
Is this print meant to stay in the final version even when no assert hits?
python/cuml/test/experimental/test_explainer_permutation_shap.py
Outdated
Show resolved
Hide resolved
Co-authored-by: John Zedlewski <904524+JohnZed@users.noreply.github.com>
Co-authored-by: John Zedlewski <904524+JohnZed@users.noreply.github.com>
Co-authored-by: John Zedlewski <904524+JohnZed@users.noreply.github.com>
Co-authored-by: John Zedlewski <904524+JohnZed@users.noreply.github.com>
rerun tests |
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.
One stray test print to remove but otherwise great!
npermutations=1, | ||
testing=True | ||
) | ||
|
||
print(shap_values) |
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.
Stray print
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.
From the failed GPU builds:
CondaValueError: invalid package specification: shap>=0.37,
This will need to be updated
@@ -58,7 +58,8 @@ gpuci_conda_retry install -c conda-forge -c rapidsai -c rapidsai-nightly -c nvid | |||
"xgboost=1.3.3dev.rapidsai${MINOR_VERSION}" \ | |||
"rapids-build-env=${MINOR_VERSION}.*" \ | |||
"rapids-notebook-env=${MINOR_VERSION}.*" \ | |||
"rapids-doc-env=${MINOR_VERSION}.*" | |||
"rapids-doc-env=${MINOR_VERSION}.*" \ | |||
"shap>=0.37,<=0.39" |
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 will include version 0.39 as well. Did you want to exclude 0.39 or include it @dantegd
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.
Include it, the original had the wrong blank space because I accidentally replaced the <=
for <
in a bulk replace when I was doing something else, thanks for the notice!
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #3596 +/- ##
===============================================
+ Coverage 80.70% 83.02% +2.31%
===============================================
Files 227 227
Lines 17615 17712 +97
===============================================
+ Hits 14217 14706 +489
+ Misses 3398 3006 -392
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@gpucibot merge |
PR isolates tests improvements from general SHAP work, and is being used for debugging SHAP in CI.