-
Notifications
You must be signed in to change notification settings - Fork 532
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
[REVIEW] Experimental versions of GPU accelerated Kernel and Permutation SHAP #3126
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
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.
I just looked at the C++/CUDA so far. Looks good to me! Will comment on python later today.
Codecov Report
@@ Coverage Diff @@
## branch-0.17 #3126 +/- ##
===============================================
+ Coverage 70.99% 72.02% +1.02%
===============================================
Files 198 205 +7
Lines 15745 16396 +651
===============================================
+ Hits 11178 11809 +631
- Misses 4567 4587 +20
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.
Looking good! I think splitting up the logic of 1-2 of the big functions would be handy. And it'd be great to add more unit tests for the utility functions, but really lookin good!
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.
Looking great! I think we're ready for a merge to experimental. I added very tiny suggestions and questions, but I think they'll be quick.
* | ||
* @param[in] handle cuML handle | ||
* @param[out] out generated data [on device] [dim = (2 * ncols * nrows_bg + nrows_bg) * ncols] | ||
* @param[in] background background data [on device] [dim = ncols * nrows_bg] |
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.
column major?
typedef MakeKSHAPDatasetTest<double> MakeKSHAPDatasetTestD; | ||
TEST_P(MakeKSHAPDatasetTestD, Result) { | ||
ASSERT_TRUE(test_sampled_X); | ||
// disabled due to a sporadic cuda 10.1 fail (by one value in one case!) |
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.
can you add an issue for this?
Co-authored-by: John Zedlewski <[email protected]>
Co-authored-by: John Zedlewski <[email protected]>
Co-authored-by: John Zedlewski <[email protected]>
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.
Looks good! I think the random state question can be deferred and improved over time.
Removing automerge label while skipping the one test that is strangely slow in CI |
No description provided.