Common knn graph build params #949
Conversation
jinsolp
added
non-breaking
| @@ -0,0 +1,78 @@ | |||
| /* | |||
There was a problem hiding this comment.
cuVS isn't header-only, so the header files are very lightweight. Instead of introducing new headers for structs and enums, can we just reuse common.hpp?
There was a problem hiding this comment.
common.hpp has the base index_params (link), so declaring graph_build_params in that file results in a circular dependency (because we make use of ivf_pq::index_params and nn_descent::index_params inside `graph_build_params)
| struct ivf_pq_params { | ||
| cuvs::neighbors::ivf_pq::index_params build_params; | ||
| cuvs::neighbors::ivf_pq::search_params search_params; | ||
| float refinement_rate = 2.0; |
There was a problem hiding this comment.
Is the default value 1 or 2?
There was a problem hiding this comment.
That's a bit tricky here... cagra uses the ivf_pq_params() constructor and expects a default refinement rate of 1.0, whereas all neighbors expects a default refinement rate of 2.0.
I agree that this is confusing. Maybe we should set the default to 1.0 and make changes from the all neighbors usage.
|
@lowener Changed defaults. Ready for another round of reviews : ) |
Reusing graph build params for cagra and all neighbors.
Related issue: #931