[REVIEW] Adding SLHC prims. #140
Conversation
cjnolet
added
improvement
|
Depends on #119 |
cjnolet
force-pushed
the
fea-019-slhc_prims
branch
from
ebe9513 to
0756bb8
Compare
cjnolet
added
3 - Ready for Review
and removed
2 - In Progress
There was a problem hiding this comment.
This is my first pass review, where I have not looked over the logic at all yet. What are your thoughts on removing the usage of raft::mr::device::buffer to favor rmm::device_uvector so we can start having new PRs not have that and eventually move towards removing our allocator completely?
| std::vector<value_idx> children_h(n_edges * 2); | ||
| std::vector<value_idx> out_size_h(n_edges); | ||
|
|
||
| uint32_t start = curTimeMillis(); |
There was a problem hiding this comment.
Adding a note for later removal
|
@divyegala, thanks for taking the time to review this! I've pushed the changes based on your review. All of the changes that were using raft device buffer objects should now be using rmm device uvector. As discussed in the review, I tried to update all the thrust functions per your request but some of them don't seem to like using the thrust policy. I don't believe any of those should be using scratch space, though, so I think we should be okay. If the remaining device pointer casts are still a concern and we can't find a quick way around them, we can always make a Github issue for it and fix it when we figure out the cause. What do you think? |
…espace conflicts for old cuda versions
|
@gpucibot merge |
Closes #3518 I've closed the original PR (#3308) which included both SLHC & HDBSCAN and opened this PR to only include the SLHC changes. This PR contains an implementation of SLHC which is currently broken across RAFT & cuML. Once we move the dense pairwise distance primitive over to RAFT the entire SLHC algorithm can live in RAFT so it can be shared w/ cugraph, and will just be exposed through cuml. If reviewing this PR, please also review the corresponding RAFT PR: rapidsai/raft#140 Authors: - Corey J. Nolet (@cjnolet) Approvers: - Divye Gala (@divyegala) - Dante Gama Dessavre (@dantegd) - Mike Wendt (@mike-wendt) URL: #3545
Forward-merge branch-24.06 into branch-24.08
The dense pairwise distances API in cuML currently relies on CUTLASS and will soon be re-written to remove that dependency. Until then, the SLHC implementation in RAFT cannot depend on that API. In the meantime, I've abstracted that piece out to make it pluggable on the cuML side.
This PR depends on the changes in #139.