[Feat] Support bitset filter for Brute Force
#560
Conversation
rhdong
added
feature request
| @@ -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.
I think we want to keep the filter immutable, don' we?
There was a problem hiding this comment.
This change is to be compatible with the bitset filter utilized in CAGRA and other algorithms. The immutable is implemented on the search API by restricting the filter to be const cuvs::neighbors::filtering::base_filter. Please refer to cagra, here
There was a problem hiding this comment.
We are using obj<const type> everywhere in the codebase for containers to immutable types so we should be doing that here as well. If the CAGRA API is not doing this, it should be.
| struct base_filter { | ||
| virtual ~base_filter() = default; | ||
| virtual ~base_filter() = default; |
There was a problem hiding this comment.
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 to bitset filter? I suspect most users will want bitset.
There was a problem hiding this comment.
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.
I believe using bitset as the default setting might not be ideal if we don't have enough input from end-users.
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.
| * be considered for each query. | ||
| * | ||
| * - Supports two types of filters: | ||
| * 1. **Bitmap Filter**: A per-query filter with a logical shape of `[n_queries, index->size()]`, |
There was a problem hiding this comment.
Can we reorder these please? I think we should be pushing bitset as the first option.
|
|
||
| namespace cuvs::neighbors::detail { | ||
|
|
||
| using namespace cuvs::neighbors::filtering; |
There was a problem hiding this comment.
Never use using namespace in a header file. It cascades to all header files. If you need to do this, do it in a source file. Otherwise, just use the absolute namespace.
|
/merge |
No description provided.