Add function for calculating the mutual_reachability_graph#323
Add function for calculating the mutual_reachability_graph#323rapids-bot[bot] merged 16 commits intorapidsai:branch-24.10from
Conversation
benfred
added
improvement
| int min_samples, | ||
| raft::device_vector_view<int64_t> indptr, | ||
| raft::device_vector_view<float> core_dists, | ||
| raft::sparse::COO<float, int64_t>& out, |
There was a problem hiding this comment.
Small nitpick- this sparse::COO class is going away completely and it's really not designed well. Can we swap this out for the raft::sparsity_owning_coo_matrix? I think we can do a view since we already know the size, right?
There was a problem hiding this comment.
It's also okay if the answer is "time is running out, let's change that later"
There was a problem hiding this comment.
One challenge here is that we'd have to add support in RAFT for device_coo_matrix_view / device_sparsity_owning_coo_matrix to some sparse algorithms like raft::sparse::linalg::symmetrize andraft::sparse::convert::sorted_coo_to_csr - which only accept the raft::sparse::COO class. This shouldn't be too hard to do though -
There was a problem hiding this comment.
Underneath the public API, we can just use the arrays / existing types. But I’d like to get to a point where we are using the new types at least for new public APIs. Eventually we need to scrape through all the sparse APIs and use the new types everywhere. However new APIs could at least use the new types in the meantime.
Can you create an issue for this? I think we are running out of time to do it for 24.10.
cjnolet
left a comment
cjnolet
left a comment
There was a problem hiding this comment.
Approving because my only comment is asking to create an issue or tracker issue for consolidating templates.
| bool select_min, | ||
| bool sorted = false, | ||
| SelectAlgo algo = SelectAlgo::kAuto, | ||
| std::optional<raft::device_vector_view<const int, int64_t>> len_i = std::nullopt); |
There was a problem hiding this comment.
It's saddening to see having to add this in here just for int type. Can you create an issue or maybe even a larger tracker issue with some follow-up tasks and add consolidating these type instantiations to that? Similar to the other types, it would be great if we could establish a single set of common integral types and maybe even consolidate float/double just into float.
There was a problem hiding this comment.
I've added an issue here #370 for consolidating the template params -
| const DistanceT* precomputed_search_norms = nullptr, | ||
| const uint32_t* filter_bitmap = nullptr) | ||
| const uint32_t* filter_bitmap = nullptr, | ||
| DistanceEpilogue distance_epilogue = raft::identity_op()) |
There was a problem hiding this comment.
Unfortunate we have to instantiate bfknn twice now :-( but great that we don't have to expose the eilogue through the public APIs. Hopefully at some point soon we'll establish a good way to specify these (maybe as JIT compiled functions) through the public APIs where we don't need to instanaite all the kernels end to end for each one.
|
/merge |
2 participants
No description provided.