-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Integrate IVF-PQ from RAFT #3044
Conversation
…ed to propagate kmeans info down into quantized index
order for FAISS integration to work successfully.
…izer and ivf lists
WIP: Implement some helpers
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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 @tarang-jain for the updates, a few additional comments.
faiss/gpu/GpuIndexIVF.cu
Outdated
cp.niter = 10; | ||
} else { | ||
// set the number of iterations to RAFT's default for IVF methods | ||
cp.niter = 20; |
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.
This change has been made.
I still see the change here, could you confirm whether it is a merge error, or intentional?
I believe it would more appropriate to reverted back to set cp.niter = 10;
unconditionally.
resources_->getRaftHandleCurrentDevice(); | ||
raft::neighbors::ivf_pq::search_params pams; | ||
pams.n_probes = nprobe; | ||
pams.lut_dtype = useFloat16LookupTables_ ? CUDA_R_16F : CUDA_R_32F; |
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.
Created tracking issue #3249
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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 @tarang-jain for the update, the PR looks good to me!
… into raft_integration
… raft_integration
@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@algoriddle merged this pull request in 27b1055. |
Imports changes from #3133 and #3171. So this single PR adds all the changes together.