Skip to content

ReachabilityPostProcess distance epilogue for NN Descent#1073

Merged
rapids-bot[bot] merged 13 commits intorapidsai:branch-25.08from
jinsolp:nnd-dist-epilogue
Jul 8, 2025
Merged

ReachabilityPostProcess distance epilogue for NN Descent#1073
rapids-bot[bot] merged 13 commits intorapidsai:branch-25.08from
jinsolp:nnd-dist-epilogue

Conversation

@jinsolp
Copy link
Copy Markdown
Contributor

@jinsolp jinsolp commented Jun 30, 2025

NN Descent changed to support distance epilogues. Currently supporting ReachabilityPostProcess and identity_op. A new distance epilogue will need new instantiations.

Mutual reachability computation will eventually be hidden behind the all_neighbors API. Basic tests are still added in this PR to ensure the correctness of this feature.

@jinsolp jinsolp self-assigned this Jun 30, 2025
@jinsolp jinsolp requested a review from a team as a code owner June 30, 2025 22:28
@jinsolp jinsolp added feature request New feature or request non-breaking Introduces a non-breaking change labels Jun 30, 2025
@github-actions github-actions Bot added the cpp label Jun 30, 2025
* limitations under the License.
*/

#pragma once
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a reason to hold up this PR, but since cuVS is no longer header-only, we don't have to have these _types files anymore. We could just include this in the shared code (assuming there is a shared place to put it).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placed it back in reachability.cuh.

The mutual reachability function will be cleaned up in another PR, and the reachability.cuh file will eventually be left with only ReachabilityPostProcess struct.

Copy link
Copy Markdown
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but before we merge this we should take a look at the binary size just to make sure we're not adding a ton of size to it.

@jinsolp
Copy link
Copy Markdown
Contributor Author

jinsolp commented Jul 3, 2025

Compared to the nightly;

  • nn_descent_float.cu: 2.941MB ->3.02MB

So looks okay : )

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Jul 8, 2025

/merge

@rapids-bot rapids-bot Bot merged commit 23c6dc0 into rapidsai:branch-25.08 Jul 8, 2025
53 checks passed
@cjnolet cjnolet moved this to In Progress in Unstructured Data Processing Jul 8, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Unstructured Data Processing Jul 8, 2025
@jinsolp jinsolp deleted the nnd-dist-epilogue branch July 8, 2025 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cpp feature request New feature or request non-breaking Introduces a non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants