cuml.experimental SHAP improvements #3433
Conversation
…18-fea-kshap-opt
dantegd
added
2 - In Progress
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #3433 +/- ##
===============================================
+ Coverage 71.48% 71.55% +0.06%
===============================================
Files 207 212 +5
Lines 16748 17082 +334
===============================================
+ Hits 11973 12223 +250
- Misses 4775 4859 +84
Continue to review full report at Codecov.
|
|
rerun tests |
3 similar comments
|
rerun tests |
|
rerun tests |
|
rerun tests |
There was a problem hiding this comment.
Looks good! I had only small suggestions, mostly stuff that can be deferred to future PRs since this is still experimental. My only concern is that the number of variations supported in datatypes (sklearn model with pandas background data or cuml with f-ordered numpy or ...) makes it hard to test all paths of the base shap initialization. Let's look at codecov there for additional test ideas and be open to simplifying the options if necessary.
|
|
||
| void shap_main_effect_dataset "ML::Explainer::shap_main_effect_dataset"( | ||
| const handle_t& handle, | ||
| float* dataset, |
There was a problem hiding this comment.
(in the underlying API) should dataset be const?
There was a problem hiding this comment.
dataset is where the output is generated, maybe I should change the name to avoid the confusion?
| ---------- | ||
| model : function | ||
| Function that takes a matrix of samples (n_samples, n_features) and | ||
| computes the output for those samples with shape (n_samples). Function |
There was a problem hiding this comment.
Ah, bummer so there is no way to use the tags api because we need to take the function rather than the model
There was a problem hiding this comment.
already use the tags API by getting the owning object of the function (if it exists) and getting tags from that:
| default=np.float32) | ||
| else: | ||
| if dtype in [np.float32, np.float64]: | ||
| self.dtype = np.dtype(dtype) |
There was a problem hiding this comment.
out of curiosity why do you have to convert to np.dtype?
There was a problem hiding this comment.
I was doing the wrong order of things, I use the dtype function of numpy so that we accept string description of the dtypes without additional work
| @@ -213,13 +198,17 @@ class SHAPBase(): | |||
| ) | |||
| ) | |||
|
|
|||
| # public attribute saved as NumPy for compatibility with the legacy | |||
| # SHAP potting functions | |||
| self.expected_value = cp.asnumpy(self._expected_value) | |||
There was a problem hiding this comment.
This makes sense, but it's a deviation from our standard approach... can you add a docstring to explain this? Can be a follow up PR.
Also would be really good to have a test of compatibility with SHAP plotting so we never break this (again, follow on PR ok)
…d other small fixes
|
rerun tests |
| // gemv, which could cause a very sporadic race condition in Pascal and | ||
| // Turing GPUs that caused it to give the wrong results. Details: | ||
| // https://github.com/rapidsai/cuml/issues/1739 | ||
| rmm::device_uvector<math_t> tmp_vector(n_cols, stream); |
There was a problem hiding this comment.
how about something like tmp_gemv_result or otherwise indicating its use?
| def output_list_shap_values(X, dimensions, output_type): | ||
| if output_type == 'cupy': | ||
| if dimensions == 1: | ||
| return X[0] | ||
| else: | ||
| return X | ||
| res = [] |
There was a problem hiding this comment.
Super picky but this seems like either a list comprehension or just list(X) would be nicer
| @@ -399,6 +416,11 @@ def test_l1_regularization(exact_tests_dataset, l1_type): | |||
| 0.00088981] | |||
| ] | |||
|
|
|||
| housing_regression_result = np.array( | |||
There was a problem hiding this comment.
Was this obtained by running shap? Would be good to note in a comment what you did to get it and what version you used.
|
rerun tests |
|
@gpucibot merge |
Closes #1739
Addresses most items of #3224