Moving random ball cover #218
Conversation
cjnolet
added
improvement
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-25.06 #218 +/- ##
=============================================
Coverage 84.49% 84.49%
=============================================
Files 20 20
Lines 129 129
=============================================
Hits 109 109
Misses 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,205 @@ | |||
| /* | |||
There was a problem hiding this comment.
nitpick: since we're no longer header only - I'd prefer to get rid of having -ext.cuh and -inl.cuh header files.
There was a problem hiding this comment.
Do you mean just consolidate them into one file or do you mean remove the intantiations / ignored extern templates?
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Linking #110 |
|
/ok to test |
|
As of commit 854837c, the total size added is now ~19 MB, while the wheel size increases from ~900 MB to ~911 MB. 
|
| * @param[inout] index an empty (and not previous built) instance of | ||
| * cuvs::neighbors::ball_cover::index | ||
| */ | ||
| void build(raft::resources const& handle, index<int64_t, float>& index); |
There was a problem hiding this comment.
Just a note- we should have the factory function here too. We can do as a follow-on at some point. RBC isn't a heavily used API today.
| * many datasets can still have great recall even by only | ||
| * looking in the closest landmark. | ||
| */ | ||
| void all_knn_query(raft::resources const& handle, |
There was a problem hiding this comment.
Eventually we should move this behind our all_neighbors API cc @jinsolp
| "This notebook demonstrates how to run approximate nearest neighbor search using cuVS IVF-Flat algorithm.\n", | ||
| "It builds and searches an index using a dataset from the ann-benchmarks million-scale datasets, saves/loads the index to disk, and explores important parameters for fine-tuning the search performance and accuracy of the index." | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Not sure if this change is valid. Do you mind looking at this notebook real quick just to verify this isn't stale?
cpp/src/neighbors/ball_cover/detail/ball_cover/registers_00_generate.py
Outdated
Show resolved
Hide resolved
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Approving, with the understanding that while this increases the binary size for cuVS a bit, it leads to a much larger compensating reduction in cuML binary sizes: rapidsai/cuml#6626 (comment)
|
/merge |
github-project-automation
bot
moved this from In Progress
to Done
in Vector Search, ML, & Data Mining Release Board
Depends on rapidsai/cuvs#218. This PR reduces the supported combination of types for `RBC` method in `dbscan.cu` to only `<float, int64_t>`. This is because this is the only type combination that cuVS compiles RBC for, which is otherwise very expensive and slow to compile. ### Effects on Binary Size Tracked here #6626 (comment) Authors: - Divye Gala (https://github.com/divyegala) Approvers: - Simon Adorf (https://github.com/csadorf) - Dante Gama Dessavre (https://github.com/dantegd) URL: #6644
Closes #135
This PR brings in RBC implementation from RAFT while also reducing the number of templates that are instantiated by moving the following templates to runtime parameters:
dimsNotes for Reviewers
Benefits of these changes
Allows for a reduction in cuML binary sizes, once cuML switches from RAFT's implementation to this one.
See:
rapidsai/cuml#6626 (comment)