-
Notifications
You must be signed in to change notification settings - Fork 564
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
[FEA] Add GPUTreeSHAP to cuML explainer module (experimental) #4351
Conversation
Locally, I'm getting RF tests outside of the set tolerance:
|
Should we put this in the experimental namespace for 21.12? |
cpp/src/explainer/tree_shap.cu
Outdated
|
||
template <typename ThresholdType, typename LeafType> | ||
ExtractedPathHandle | ||
extract_paths_impl(const tl::ModelImpl<ThresholdType, LeafType>& model) { |
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.
The logic of this function seems generally correct, but it's also a bit hard to follow. Could we break it up a bit or consider using algorithms directly on the underlying containers where feasible rather than trying to keep track of all the separate indexes?
I fixed the failing unit test. Marking this PR as non-draft, since it's functionally complete. I'll now address stylistic concerns. |
ExtractedPathImpl<ThresholdType>* paths | ||
= dynamic_cast<ExtractedPathImpl<ThresholdType>*>(path_container.get()); | ||
std::unique_ptr<TreePathInfo> path_info_ptr = std::make_unique<TreePathInfoImpl<ThresholdType>>(); | ||
auto* path_info = dynamic_cast<TreePathInfoImpl<ThresholdType>*>(path_info_ptr.get()); |
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 think it would be better to avoid the dynamic_cast
here (and the general complexity of the inner implementation classes for different types) by using a single struct with a std::variant
member variable, but let's just make note of that and save it for a follow-on.
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #4351 +/- ##
===============================================
Coverage ? 85.65%
===============================================
Files ? 236
Lines ? 19179
Branches ? 0
===============================================
Hits ? 16427
Misses ? 2752
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@gpucibot merge |
Addresses rapidsai#4110 This is an experimental prototype. For now, it supports: * XGBoost models with numerical splits * cuML RF regressors with numerical splits cuML RF classifiers are not supported. Authors: - Philip Hyunsu Cho (https://github.com/hcho3) Approvers: - Rory Mitchell (https://github.com/RAMitchell) - William Hicks (https://github.com/wphicks) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#4351
Addresses #4110
This is an experimental prototype. For now, it supports:
cuML RF classifiers are not supported.