-
Notifications
You must be signed in to change notification settings - Fork 197
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
Some MST updates for single-linkage / HDBSCAN clustering #119
Conversation
This PR has been labeled |
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.
We can adjust the tests as discussed offline, and this should be good to go!
detail::add_reverse_edge<<<nblocks, nthreads, 0, stream>>>( | ||
new_mst_edge_ptr, indices, weights, temp_src_ptr, temp_dst_ptr, | ||
temp_weights_ptr, v); | ||
// detail::add_reverse_edge<<<nblocks, nthreads, 0, stream>>>( |
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.
An extra comment here to document why this kernel launch is commented
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 had forgotten about this change. @afender, this will probably break cugraph if we remove the symmetry altogether right? Should we make this optional?
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.
If this is made optional, then the changes to the above additions will also have to be made optional
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.
Just to make note here- we talked offline about this. @divyegala will push necessary updates directly to this branch (or open a follow-on if necessary). SLHC is dependent upon this change but we can't break cugraph's assumption of symmetric outputs.
The rust api docs on docs.rapids.ai are deployed to https://docs.rapids.ai/api/cuvs/nightly/rust_api/index.html but the relative link in the iframe was assuming that these were at https://docs.rapids.ai/api/cuvs/nightly/rust_api.html. (There seems to be some difference in the url structure depending on whether you build locally or not) Fix by forcing the url structure to always be `rust_api/index.html` Authors: - Ben Frederickson (https://github.com/benfred) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai/cuvs#119
We may not need this for single-linkage / hdbscan clustering. Not sure yet.