-
Notifications
You must be signed in to change notification settings - Fork 68
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
Inner Product for CAGRA-Q #458
base: branch-24.12
Are you sure you want to change the base?
Conversation
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 for the PR. It's nice to see only minimal changes are needed to add a new metric.
There are (as expected) many new files though. Could you please check the binary size doesn't blow up?
cpp/test/neighbors/ann_cagra.cuh
Outdated
{false}, | ||
{true}, | ||
{0.6}); // don't demand high recall without refinement | ||
{0.55}); // don't demand high recall without 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.
Please justify, why reducing the recall here? Why is it not a bug in build or search functions?
half2 dist = dist_op<half2, DescriptorT::kMetric>( | ||
q2, c2 + reinterpret_cast<half2(&)[PQ_LEN * vlen / 2]>(vq_vals)[d1]); |
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.
Please benchmark this before/after the change at least for: single_cta/multi_cta, itopk in range of 32...512, with couple PQ configs. I'm concerned specifically about a possibility of increased register usage and spilling, which could manifest in a significant drop in QPS in some cases.
Partially addresses #198 (Cosine still pending for CAGRA and CAGRA-Q).