Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Migrate feature diff for NN Descent from RAFT to cuVS #421
Migrate feature diff for NN Descent from RAFT to cuVS #421
Changes from 7 commits
09e1d73
c29be2d
e13bd0c
5ab72c3
3e084d3
252a62d
fdea317
2172a84
d2d77d7
47ef5b5
1976f76
9604963
7846a64
3532c22
cb52674
c98eed5
ccb0c5a
14680b9
4d92530
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why doesn't nn-descent return the built index like all the other index types?
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.
It does, there's an API for that as well. We need this API as well especially for CAGRA because it needs to own the knn graph that it sends to NN Descent. For that, it needs to construct an index first and that's why we need this API.
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.
But the usage docs for this function don't reflect that. The whole idea behind these
build
functon is to abstract away the index so that the user (and anyone using the public APIs) don't need to think about the underlying index object. Instead of having the user construct the index object on their own, we shuold have them pass an optional knn graph into thebuild
function that it then uses when it constructs the underlying index instance underneath.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.
Done in latest commit
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.
Make sure all of these are exposed through the docs.
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.
Yes, they are part of the doxygen group that is already in the docs source.