[FEA] Binary IVF Flat Index - #1099
Open
tarang-jain wants to merge 163 commits into
Open
tarang-jain wants to merge 163 commits into
tarang-jain wants to merge 163 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
tarang-jain
added
feature request
…nto binary-kmeans
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
cpp/src/distance/detail/fused_distance_nn/simt_kernel.cuh (1)
88-88: 💤 Low valueRedundant
AccTtype alias definition.
AccTis defined identically at line 88 and again at line 155. Consider removing the duplicate at line 155 since the type alias is already in scope.♻️ Proposed fix
- using AccT = std::conditional_t<std::is_same_v<DataT, uint8_t>, uint32_t, DataT>; PairwiseDistances<DataT, - AccT, // OutT (unused in PairwiseDistances) + AccT, // OutT (unused in PairwiseDistances) - AccT defined at line 88 IdxT,Also applies to: 155-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/distance/detail/fused_distance_nn/simt_kernel.cuh` at line 88, The type alias AccT (defined as std::conditional_t<std::is_same_v<DataT, uint8_t>, uint32_t, DataT>) is declared twice; remove the redundant second declaration so only the first AccT definition remains in scope. Locate the duplicate AccT declaration (the one identical to the existing alias) and delete it, ensuring no other symbols (e.g., DataT) lose scope or require a re-declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cuvs/neighbors/ivf_flat.hpp`:
- Line 273: Add a complete Doxygen comment block for the public method
binary_index() describing what the method does, its return semantics (what
true/false means), that it is noexcept, and any side effects or invariants it
relies on; update the declaration bool binary_index() const noexcept; with a
Doxygen block that includes a brief description, a `@return` explaining the
boolean meaning, and notes about thread-safety or state changes if applicable to
the IVF_FLAT index class.
In `@cpp/src/cluster/detail/kmeans_balanced.cuh`:
- Around line 1176-1180: The raft::copy call that copies centers into
cluster_centers uses dim for both the source offset and length but in packed
binary mode the source mc_trainset_ccenters is stored in expanded space and must
use transformed_dim; update the offset arithmetic and copy length to use
transformed_dim (e.g., replace uses of dim in the raft::make_device_vector_view
calls with transformed_dim) so the copy uses fine_clusters_nums[i] *
transformed_dim and the start pointer uses (fine_clusters_csum[i] *
transformed_dim), ensuring mc_trainset_ccenters, fine_clusters_nums,
fine_clusters_csum, transformed_dim and cluster_centers are addressed
consistently for packed binary mode.
- Around line 516-518: The function predict uses transformed_dim but never
defines it; add a local definition named transformed_dim near the start of
predict (after line 515) that captures the post-transformation feature dimension
used by the rest of the code (the size used by transform/cur_dataset allocation
and minibatch handling). Initialize transformed_dim from the existing metadata
or helper used elsewhere in this file (for example use the transformed
feature-count accessor if one exists, or fall back to the input feature count
n_features when no transform is applied) so that calls referencing
transformed_dim (cur_dataset allocation, max_minibatch_size * transformed_dim,
and other usages) compile and use the correct dimension.
In `@cpp/src/distance/detail/fused_distance_nn/fused_bitwise_hamming_nn.cuh`:
- Around line 45-46: The min-initializer uses maxVal from DataT (uint8_t) which
is too small for accumulated BitwiseHamming distances; replace the sentinel with
the maximum value of the accumulator type (uint32_t) instead of
std::numeric_limits<DataT>::max() so that initial mins (used in functions
referenced by kv_pair_type and fused_bitwise_hamming_nn) are correct for k>31,
and update the caller-side output-buffer initialization for the BitwiseHamming
path to use the same accumulator-range sentinel (uint32_t max) so both producer
and consumer use the same range.
In `@cpp/src/neighbors/ivf_flat/ivf_flat_build.cuh`:
- Around line 228-248: The centroids_view for binary centers is created
unconditionally from index->binary_centers() even when index->binary_index() is
false; move the raft::make_device_matrix_view<const uint8_t, IdxT> construction
for centroids_view inside the if (index->binary_index()) branch and only call
cuvs::cluster::kmeans::detail::predict_bitwise_hamming(handle, batch_data_view,
centroids_view, batch_labels_view) there; leave the non-binary branch to
construct orig_centroids_view from index->centers() and call
cuvs::cluster::kmeans_balanced::predict(...) as-is to avoid creating/using an
invalid binary_centroids view.
---
Nitpick comments:
In `@cpp/src/distance/detail/fused_distance_nn/simt_kernel.cuh`:
- Line 88: The type alias AccT (defined as
std::conditional_t<std::is_same_v<DataT, uint8_t>, uint32_t, DataT>) is declared
twice; remove the redundant second declaration so only the first AccT definition
remains in scope. Locate the duplicate AccT declaration (the one identical to
the existing alias) and delete it, ensuring no other symbols (e.g., DataT) lose
scope or require a re-declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0e667e0-a114-4f9a-9b44-e4171c233dbf
📒 Files selected for processing (23)
cpp/include/cuvs/cluster/kmeans.hppcpp/include/cuvs/detail/jit_lto/ivf_flat/interleaved_scan_fragments.hppcpp/include/cuvs/neighbors/ivf_flat.hppcpp/src/cluster/detail/kmeans_balanced.cuhcpp/src/cluster/kmeans_balanced.cuhcpp/src/distance/detail/distance_ops/all_ops.cuhcpp/src/distance/detail/distance_ops/bitwise_hamming.cuhcpp/src/distance/detail/fused_distance_nn.cuhcpp/src/distance/detail/fused_distance_nn/fused_bitwise_hamming_nn.cuhcpp/src/distance/detail/fused_distance_nn/helper_structs.cuhcpp/src/distance/detail/fused_distance_nn/simt_kernel.cuhcpp/src/distance/detail/pairwise_matrix/dispatch-ext.cuhcpp/src/distance/detail/pairwise_matrix/dispatch_matrix.jsoncpp/src/distance/fused_distance_nn-inl.cuhcpp/src/neighbors/detail/ann_utils.cuhcpp/src/neighbors/ivf_flat/detail/jit_lto_kernels/metric_impl.cuhcpp/src/neighbors/ivf_flat/detail/jit_lto_kernels/metric_matrix.jsoncpp/src/neighbors/ivf_flat/ivf_flat_build.cuhcpp/src/neighbors/ivf_flat/ivf_flat_interleaved_scan_jit.cuhcpp/src/neighbors/ivf_flat/ivf_flat_search.cuhcpp/src/neighbors/ivf_flat_index.cppcpp/tests/neighbors/ann_ivf_flat.cuhcpp/tests/neighbors/ann_utils.cuh
|
|
||
| void check_consistency(); | ||
|
|
||
| bool binary_index() const noexcept; |
There was a problem hiding this comment.
Add Doxygen docs for the new public binary_index() API.
This new public method should include a complete Doxygen block (at least behavior/return semantics) to match the header documentation standard.
📝 Proposed doc update
+ /**
+ * `@brief` Indicates whether this index is configured for binary (BitwiseHamming) mode.
+ *
+ * `@return` true if binary centers are used; false otherwise.
+ */
bool binary_index() const noexcept;As per coding guidelines: cpp/include/cuvs/**/*.{hpp,h}: "All public API functions must include complete Doxygen documentation describing parameters, return values, and any side effects".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool binary_index() const noexcept; | |
| /** | |
| * `@brief` Indicates whether this index is configured for binary (BitwiseHamming) mode. | |
| * | |
| * `@return` true if binary centers are used; false otherwise. | |
| */ | |
| bool binary_index() const noexcept; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/include/cuvs/neighbors/ivf_flat.hpp` at line 273, Add a complete Doxygen
comment block for the public method binary_index() describing what the method
does, its return semantics (what true/false means), that it is noexcept, and any
side effects or invariants it relies on; update the declaration bool
binary_index() const noexcept; with a Doxygen block that includes a brief
description, a `@return` explaining the boolean meaning, and notes about
thread-safety or state changes if applicable to the IVF_FLAT index class.
Comment on lines
+516
to
+518
| rmm::device_uvector<MathT> cur_dataset( | ||
| std::is_same_v<T, MathT> ? 0 : max_minibatch_size * dim, stream, mem_res); | ||
| bool need_compute_norm = | ||
| dataset_norm == nullptr && (params.metric == cuvs::distance::DistanceType::L2Expanded || | ||
| params.metric == cuvs::distance::DistanceType::L2SqrtExpanded || | ||
| params.metric == cuvs::distance::DistanceType::CosineExpanded); | ||
| std::is_same_v<T, MathT> ? 0 : max_minibatch_size * transformed_dim, stream, mem_res); | ||
| bool need_compute_norm = dataset_norm == nullptr && |
There was a problem hiding this comment.
Critical: transformed_dim is undefined in this function.
The variable transformed_dim is used at lines 517, 529, 542, and 583, but it is never defined within the predict function. This will cause a compilation error.
🐛 Proposed fix
Add the variable definition near the start of the function (after line 515):
auto [max_minibatch_size, _mem_per_row] = calc_minibatch_size<MathT>(
handle, n_clusters, n_rows, dim, params.metric, std::is_same_v<T, MathT>);
+ IdxT transformed_dim = params.is_packed_binary ? dim * 8 : dim;
rmm::device_uvector<MathT> cur_dataset(
std::is_same_v<T, MathT> ? 0 : max_minibatch_size * transformed_dim, stream, mem_res);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rmm::device_uvector<MathT> cur_dataset( | |
| std::is_same_v<T, MathT> ? 0 : max_minibatch_size * dim, stream, mem_res); | |
| bool need_compute_norm = | |
| dataset_norm == nullptr && (params.metric == cuvs::distance::DistanceType::L2Expanded || | |
| params.metric == cuvs::distance::DistanceType::L2SqrtExpanded || | |
| params.metric == cuvs::distance::DistanceType::CosineExpanded); | |
| std::is_same_v<T, MathT> ? 0 : max_minibatch_size * transformed_dim, stream, mem_res); | |
| bool need_compute_norm = dataset_norm == nullptr && | |
| IdxT transformed_dim = params.is_packed_binary ? dim * 8 : dim; | |
| rmm::device_uvector<MathT> cur_dataset( | |
| std::is_same_v<T, MathT> ? 0 : max_minibatch_size * transformed_dim, stream, mem_res); | |
| bool need_compute_norm = dataset_norm == nullptr && |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/cluster/detail/kmeans_balanced.cuh` around lines 516 - 518, The
function predict uses transformed_dim but never defines it; add a local
definition named transformed_dim near the start of predict (after line 515) that
captures the post-transformation feature dimension used by the rest of the code
(the size used by transform/cur_dataset allocation and minibatch handling).
Initialize transformed_dim from the existing metadata or helper used elsewhere
in this file (for example use the transformed feature-count accessor if one
exists, or fall back to the input feature count n_features when no transform is
applied) so that calls referencing transformed_dim (cur_dataset allocation,
max_minibatch_size * transformed_dim, and other usages) compile and use the
correct dimension.
Comment on lines
+1176
to
+1180
| raft::copy(handle, | ||
| raft::make_device_vector_view(cluster_centers + (dim * fine_clusters_csum[i]), | ||
| fine_clusters_nums[i] * dim), | ||
| raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(), | ||
| fine_clusters_nums[i] * dim)); | ||
| raft::make_device_vector_view(cluster_centers + (dim * fine_clusters_csum[i]), | ||
| fine_clusters_nums[i] * dim), | ||
| raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(), | ||
| fine_clusters_nums[i] * dim)); |
There was a problem hiding this comment.
Incorrect copy size for packed binary mode.
For packed binary mode, mc_trainset_ccenters contains centers in expanded space (fine_clusters_nums_max * transformed_dim elements, per line 1084). However, the copy uses dim instead of transformed_dim for both the offset and size, which will copy the wrong amount of data and corrupt the output.
🐛 Proposed fix
- raft::copy(handle,
- raft::make_device_vector_view(cluster_centers + (dim * fine_clusters_csum[i]),
- fine_clusters_nums[i] * dim),
- raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(),
- fine_clusters_nums[i] * dim));
+ IdxT centers_stride = params.is_packed_binary ? transformed_dim : dim;
+ raft::copy(handle,
+ raft::make_device_vector_view(cluster_centers + (centers_stride * fine_clusters_csum[i]),
+ fine_clusters_nums[i] * centers_stride),
+ raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(),
+ fine_clusters_nums[i] * centers_stride));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| raft::copy(handle, | |
| raft::make_device_vector_view(cluster_centers + (dim * fine_clusters_csum[i]), | |
| fine_clusters_nums[i] * dim), | |
| raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(), | |
| fine_clusters_nums[i] * dim)); | |
| raft::make_device_vector_view(cluster_centers + (dim * fine_clusters_csum[i]), | |
| fine_clusters_nums[i] * dim), | |
| raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(), | |
| fine_clusters_nums[i] * dim)); | |
| IdxT centers_stride = params.is_packed_binary ? transformed_dim : dim; | |
| raft::copy(handle, | |
| raft::make_device_vector_view(cluster_centers + (centers_stride * fine_clusters_csum[i]), | |
| fine_clusters_nums[i] * centers_stride), | |
| raft::make_device_vector_view<const MathT>(mc_trainset_ccenters.data(), | |
| fine_clusters_nums[i] * centers_stride)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/cluster/detail/kmeans_balanced.cuh` around lines 1176 - 1180, The
raft::copy call that copies centers into cluster_centers uses dim for both the
source offset and length but in packed binary mode the source
mc_trainset_ccenters is stored in expanded space and must use transformed_dim;
update the offset arithmetic and copy length to use transformed_dim (e.g.,
replace uses of dim in the raft::make_device_vector_view calls with
transformed_dim) so the copy uses fine_clusters_nums[i] * transformed_dim and
the start pointer uses (fine_clusters_csum[i] * transformed_dim), ensuring
mc_trainset_ccenters, fine_clusters_nums, fine_clusters_csum, transformed_dim
and cluster_centers are addressed consistently for packed binary mode.
Comment on lines
+45
to
+46
| constexpr auto maxVal = std::numeric_limits<DataT>::max(); | ||
| using kv_pair_type = raft::KeyValuePair<IdxT, uint32_t>; |
There was a problem hiding this comment.
Use an accumulator-range sentinel for min initialization.
maxVal is derived from DataT (uint8_t), so it initializes to 255. Bitwise Hamming here accumulates into uint32_t; for k > 31, true distances can exceed 255, causing incorrect min selection.
Suggested fix
- constexpr auto maxVal = std::numeric_limits<DataT>::max();
+ constexpr uint32_t maxVal = std::numeric_limits<uint32_t>::max();Please also align the caller-side output-buffer initialization for the BitwiseHamming path to the same range.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constexpr auto maxVal = std::numeric_limits<DataT>::max(); | |
| using kv_pair_type = raft::KeyValuePair<IdxT, uint32_t>; | |
| constexpr uint32_t maxVal = std::numeric_limits<uint32_t>::max(); | |
| using kv_pair_type = raft::KeyValuePair<IdxT, uint32_t>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/distance/detail/fused_distance_nn/fused_bitwise_hamming_nn.cuh`
around lines 45 - 46, The min-initializer uses maxVal from DataT (uint8_t) which
is too small for accumulated BitwiseHamming distances; replace the sentinel with
the maximum value of the accumulator type (uint32_t) instead of
std::numeric_limits<DataT>::max() so that initial mins (used in functions
referenced by kv_pair_type and fused_bitwise_hamming_nn) are correct for k>31,
and update the caller-side output-buffer initialization for the BitwiseHamming
path to use the same accumulator-range sentinel (uint32_t max) so both producer
and consumer use the same range.
Comment on lines
+228
to
+248
| auto centroids_view = raft::make_device_matrix_view<const uint8_t, IdxT>( | ||
| index->binary_centers().data_handle(), n_lists, dim); | ||
|
|
||
| if (index->binary_index()) { | ||
| if constexpr (std::is_same_v<T, uint8_t>) { | ||
| cuvs::cluster::kmeans::detail::predict_bitwise_hamming( | ||
| handle, batch_data_view, centroids_view, batch_labels_view); | ||
| } else { | ||
| RAFT_FAIL("BitwiseHamming distance is only supported with uint8_t data type, got %s", | ||
| typeid(T).name()); | ||
| } | ||
| } else { | ||
| auto orig_centroids_view = raft::make_device_matrix_view<const float, IdxT>( | ||
| index->centers().data_handle(), n_lists, dim); | ||
| cuvs::cluster::kmeans_balanced::predict(handle, | ||
| kmeans_params, | ||
| batch_data_view, | ||
| orig_centroids_view, | ||
| batch_labels_view, | ||
| utils::mapping<float>{}); | ||
| } |
There was a problem hiding this comment.
Move binary centroids view construction inside the conditional block.
centroids_view is created from binary_centers() at lines 228-229 before checking index->binary_index(). For non-binary indexes, accessing binary_centers() may return an uninitialized or empty view. The view should be created only when needed.
🐛 Proposed fix
- auto centroids_view = raft::make_device_matrix_view<const uint8_t, IdxT>(
- index->binary_centers().data_handle(), n_lists, dim);
-
if (index->binary_index()) {
if constexpr (std::is_same_v<T, uint8_t>) {
+ auto centroids_view = raft::make_device_matrix_view<const uint8_t, IdxT>(
+ index->binary_centers().data_handle(), n_lists, dim);
cuvs::cluster::kmeans::detail::predict_bitwise_hamming(
handle, batch_data_view, centroids_view, batch_labels_view);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto centroids_view = raft::make_device_matrix_view<const uint8_t, IdxT>( | |
| index->binary_centers().data_handle(), n_lists, dim); | |
| if (index->binary_index()) { | |
| if constexpr (std::is_same_v<T, uint8_t>) { | |
| cuvs::cluster::kmeans::detail::predict_bitwise_hamming( | |
| handle, batch_data_view, centroids_view, batch_labels_view); | |
| } else { | |
| RAFT_FAIL("BitwiseHamming distance is only supported with uint8_t data type, got %s", | |
| typeid(T).name()); | |
| } | |
| } else { | |
| auto orig_centroids_view = raft::make_device_matrix_view<const float, IdxT>( | |
| index->centers().data_handle(), n_lists, dim); | |
| cuvs::cluster::kmeans_balanced::predict(handle, | |
| kmeans_params, | |
| batch_data_view, | |
| orig_centroids_view, | |
| batch_labels_view, | |
| utils::mapping<float>{}); | |
| } | |
| if (index->binary_index()) { | |
| if constexpr (std::is_same_v<T, uint8_t>) { | |
| auto centroids_view = raft::make_device_matrix_view<const uint8_t, IdxT>( | |
| index->binary_centers().data_handle(), n_lists, dim); | |
| cuvs::cluster::kmeans::detail::predict_bitwise_hamming( | |
| handle, batch_data_view, centroids_view, batch_labels_view); | |
| } else { | |
| RAFT_FAIL("BitwiseHamming distance is only supported with uint8_t data type, got %s", | |
| typeid(T).name()); | |
| } | |
| } else { | |
| auto orig_centroids_view = raft::make_device_matrix_view<const float, IdxT>( | |
| index->centers().data_handle(), n_lists, dim); | |
| cuvs::cluster::kmeans_balanced::predict(handle, | |
| kmeans_params, | |
| batch_data_view, | |
| orig_centroids_view, | |
| batch_labels_view, | |
| utils::mapping<float>{}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/neighbors/ivf_flat/ivf_flat_build.cuh` around lines 228 - 248, The
centroids_view for binary centers is created unconditionally from
index->binary_centers() even when index->binary_index() is false; move the
raft::make_device_matrix_view<const uint8_t, IdxT> construction for
centroids_view inside the if (index->binary_index()) branch and only call
cuvs::cluster::kmeans::detail::predict_bitwise_hamming(handle, batch_data_view,
centroids_view, batch_labels_view) there; leave the non-binary branch to
construct orig_centroids_view from index->centers() and call
cuvs::cluster::kmeans_balanced::predict(...) as-is to avoid creating/using an
invalid binary_centroids view.
Contributor
Author
|
/ok to test 7f27206 |
@tarang-jain, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
Author
|
/ok to test 7f27206 |
@tarang-jain, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
Author
|
/ok to test 7f27206 |
Contributor
Author
|
/ok to test 1b94be6 |
@tarang-jain, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
Author
|
/ok to test 759f6e2 |
Contributor
Author
|
/ok to test ec879d9 |
@tarang-jain, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
Author
|
/ok to test ec879d9 |
@tarang-jain, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
Author
|
/ok to test ec879d9 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on NVIDIA/raft#2770
Implementation of binary ivf flat index (bitwise hamming metric for the IVF Flat index)
Key Features
1. Binary Index Structure
binary_centers_field to store cluster centers as packeduint8_tarrays for binary datauint8_tinputs with BitwiseHamming and add only single instantiations of newly added kernels2. K-means Clustering for Binary Data
The clustering approach for binary data required special handling:
Expanded Space Clustering: Binary data (uint8_t) is expanded to signed representation (int8_t) where each bit becomes ±1
Centroid Quantization: After computing float centroids in expanded space, they are converted back to binary format:
3. Distance Kernels
Coarse Search (Cluster Selection)
bitwise_hamming_distance_opfor query-to-centroid distances in order to computePairwiseDistancesFine-Grained Search (Within Clusters)
Extended the interleaved scan kernel (
ivf_flat_interleaved_scan.cuh) with specialized templates for BitwiseHamming:Veclen-based optimization: Different code paths based on vectorization width
uint32_t, use__popc(x ^ y)for 4-byte Hamming distanceEfficient memory access patterns:
loadAndComputeDisttemplates foruint8_tthat leverage vectorized loadsas of 10/17/2025
Binary size increase:
branch-25.12 (CUDA 12.9 + X86): 1232.414 MB
This PR (CUDA 12.9 + X86): 1251.051 MB