-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add refine
to public API
#154
Conversation
Signed-off-by: Mickael Ide <[email protected]>
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.
Thanks Mickael for the PR! Looks good, just a few small comments.
void refine(raft::resources const& handle, | ||
raft::host_matrix_view<const float, int64_t, raft::row_major> dataset, | ||
raft::host_matrix_view<const float, int64_t, raft::row_major> queries, | ||
raft::host_matrix_view<const uint32_t, int64_t, raft::row_major> neighbor_candidates, |
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.
Note: this is the only place where uint32_t indices are accepted. While I think it is good to handle uint32_t indices, if we do that we shall probably do for all dataset type (not only floats).
We should agree on what the supported index type shall be, and update here accordingly. I have created an issue for discussion #156.
Co-authored-by: Tamas Bela Feher <[email protected]>
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.
Looks great- just one thing wrt docs (and we should really make sure we're doing it across the public APIs).
/** | ||
* @defgroup ann_refine Approximate Nearest Neighbors Refinement | ||
* @{ | ||
*/ |
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 know it seems exhaustive and repetitive, but for documentation's sake, please copy and update the docs for each individual prototype below. We want to make sure what any function in the docs a user looks up is going to carry along the full doc string with it, not matter the type.
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.
Thanks @lowener for the updates, the PR looks good to me!
Signed-off-by: Mickael Ide <[email protected]>
/merge |
Add
cuvs::neighbors::refine
to public API, with it's test