-
Notifications
You must be signed in to change notification settings - Fork 79
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
[Feat] Support bitset
filter for Brute Force
#560
Changes from all commits
1ba31da
4e30bd2
cbc5d38
3a5d4e0
8a45192
e79b1e3
8c0031a
4c53846
4a53e94
85d2dfc
5ef5bc5
f53d1ce
9beb58f
36bae13
1fcc7de
b58f2a5
6c7b583
7c4d50e
3ecccfb
6cc5059
4243fb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,8 +67,8 @@ void _search(cuvsResources_t res, | |
using queries_mdspan_type = raft::device_matrix_view<T const, int64_t, QueriesLayoutT>; | ||
using neighbors_mdspan_type = raft::device_matrix_view<int64_t, int64_t, raft::row_major>; | ||
using distances_mdspan_type = raft::device_matrix_view<float, int64_t, raft::row_major>; | ||
using prefilter_mds_type = raft::device_vector_view<const uint32_t, int64_t>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to keep the filter immutable, don' we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is to be compatible with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are using |
||
using prefilter_bmp_type = cuvs::core::bitmap_view<const uint32_t, int64_t>; | ||
using prefilter_mds_type = raft::device_vector_view<uint32_t, int64_t>; | ||
using prefilter_bmp_type = cuvs::core::bitmap_view<uint32_t, int64_t>; | ||
|
||
auto queries_mds = cuvs::core::from_dlpack<queries_mdspan_type>(queries_tensor); | ||
auto neighbors_mds = cuvs::core::from_dlpack<neighbors_mdspan_type>(neighbors_tensor); | ||
|
@@ -85,14 +85,14 @@ void _search(cuvsResources_t res, | |
distances_mds, | ||
cuvs::neighbors::filtering::none_sample_filter{}); | ||
} else if (prefilter.type == BITMAP) { | ||
auto prefilter_ptr = reinterpret_cast<DLManagedTensor*>(prefilter.addr); | ||
auto prefilter_mds = cuvs::core::from_dlpack<prefilter_mds_type>(prefilter_ptr); | ||
auto prefilter_view = cuvs::neighbors::filtering::bitmap_filter( | ||
prefilter_bmp_type((const uint32_t*)prefilter_mds.data_handle(), | ||
auto prefilter_ptr = reinterpret_cast<DLManagedTensor*>(prefilter.addr); | ||
auto prefilter_mds = cuvs::core::from_dlpack<prefilter_mds_type>(prefilter_ptr); | ||
const auto prefilter = cuvs::neighbors::filtering::bitmap_filter( | ||
prefilter_bmp_type((uint32_t*)prefilter_mds.data_handle(), | ||
queries_mds.extent(0), | ||
index_ptr->dataset().extent(0))); | ||
cuvs::neighbors::brute_force::search( | ||
*res_ptr, params, *index_ptr, queries_mds, neighbors_mds, distances_mds, prefilter_view); | ||
*res_ptr, params, *index_ptr, queries_mds, neighbors_mds, distances_mds, prefilter); | ||
} else { | ||
RAFT_FAIL("Unsupported prefilter type: BITSET"); | ||
} | ||
|
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 notice no changes have been made to
brute_force.hpp
. Ideally, we'll at at least be listing out in the docs which filters are supported, right? Otherwise this is going to be very confusing for users. Also, can we set the default tobitset
filter? I suspect most users will want bitset.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’ve just added the comments. I believe using bitset as the default setting might not be ideal if we don't have enough input from end-users. Perhaps we should discuss this in the team group, as I noticed that the none filter is also set as the default in CAGRA.
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 think you may have misunderstood me. The none filter is fine as the default for the the search functions, but for the code example in the docs, we should make sure we use a bitset and leave bitmap to users who need it. FAISS doesn't even support a bitmap and users aren't asking for it generally. It's good to keep it exposed for users who might need it.