Unify the two matrix types by suri-kumkaran · Pull Request #1269 · microsoft/DiskANN

suri-kumkaran · 2026-07-21T22:36:45Z

We carry two matrix types for overlapping jobs: the dense MatrixBase in diskann-utils (the Matrix/MatrixView everything reaches for), and diskann-quantization's Repr-generic Mat<Standard<T>>, which also covers the exotic layouts (block-transposed, quantized). This PR folds them into one.

The Repr framework is the more capable of the two, so it stays — moving into diskann-utils, where the dense types already live. Dense becomes a thin alias layer (Matrix<T> = Mat<RowMajor<T>>) that keeps every existing name, so callers barely notice, and the old MatrixBase is deleted.

A few things worth flagging:

Standard is renamed to RowMajor — the codebase already says "row-major" everywhere; Standard was the odd one out.
The generic constructor Mat::new(repr, init) becomes Mat::from_repr, freeing plain new for the dense Matrix::new(gen, rows, cols) most callers use.
Quantization keeps its block-transposed and quantized layouts; they now impl diskann_utils::matrix::Repr instead of owning the framework.
No crate-root re-export in diskann-utils — it's already a grab-bag, so the types stay under diskann_utils::matrix.

How to review

Most of the diff (~110 files) is mechanical and safe to skim: Standard→RowMajor, MatrixBase→Matrix, the constructor renames, views::*→matrix::*. The parts that want real attention:

diskann-utils/src/matrix.rs — the heart of it. Git tracks it as a rename of diskann-quantization/src/multi_vector/matrix.rs, so read it as "that file, Standard→RowMajor, plus the dense aliases/API and DenseData folded in."
lib.rs, the deleted views.rs, strided.rs — the module wiring, and where MatrixBase used to live.
multi_vector/mod.rs, the deleted multi_vector/matrix.rs, block_transposed.rs — the quantization side now building on the shared framework.
product/tables/basic.rs and minmax/multi/meta.rs — the only spots with real logic changes, not mechanical ones.

…rd<T>> type

Copilot

Pull request overview

This PR unifies DiskANN’s matrix abstractions by moving the more general Repr-based Mat framework into diskann-utils::matrix and rebuilding the dense Matrix/MatrixView APIs as thin row-major aliases (Matrix<T> = Mat<RowMajor<T>>). Call sites across the workspace are updated from views::* to matrix::*, and Standard is renamed to RowMajor for consistency with existing terminology.

Changes:

Introduces diskann-utils::matrix as the shared home for Mat + Repr and the dense row-major Matrix alias layer; removes/rewires the previous views usage.
Renames Standard → RowMajor and updates constructors (Mat::new(repr, init) → Mat::from_repr(...), dense Matrix::new(gen, rows, cols) usage).
Updates quantization/table code to depend on the shared diskann_utils::matrix framework and adjusts a small number of non-mechanical invariants/paths (e.g., pivot-dimension checks).

Reviewed changes

Copilot reviewed 126 out of 126 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
diskann/src/graph/workingset/map.rs	Updates doctests/tests to import `Matrix` from `diskann_utils::matrix`.
diskann/src/graph/test/synthetic.rs	Switches graph test helpers to `diskann_utils::matrix::{Init, Matrix}`.
diskann/src/graph/test/provider.rs	Updates matrix imports to the new module.
diskann/src/graph/test/cases/grid_insert.rs	Updates `MatrixView` import path to `diskann_utils::matrix`.
diskann/src/graph/start_point.rs	Rewires `Matrix`/`MatrixView` imports from `views` → `matrix`.
diskann/src/graph/glue.rs	Updates `Batch` docs/impl to target `diskann_utils::matrix::Matrix`.
diskann/src/flat/test/provider.rs	Switches test provider to `diskann_utils::matrix::Matrix`.
diskann/src/error/ann_error.rs	Updates conversion to new `diskann_utils::matrix::TryFromError` type.
diskann-utils/src/strided.rs	Adapts strided matrix utilities to new dense view types and `DenseData` traits under `matrix`.
diskann-utils/src/sampling/medoid.rs	Updates sampling code to use `crate::matrix::MatrixView` (and tests to `Init/Matrix`).
diskann-utils/src/sampling/latin_hypercube.rs	Switches LHS sampling to `crate::matrix::{Matrix, MatrixView}` (and tests).
diskann-utils/src/matrix.rs	Adds the unified `Mat`/`Repr` framework + dense row-major aliases and constructors.
diskann-utils/src/lib.rs	Removes `views` module export and adds `pub mod matrix`.
diskann-utils/src/io.rs	Rewires IO helpers to use `crate::matrix::{Matrix, MatrixView}` (and `Init` in tests).
diskann-tools/src/utils/ground_truth.rs	Updates ground-truth utility to consume `diskann_utils::matrix::Matrix`.
diskann-quantization/tests/compile-fail/multi/matref_rows.stderr	Mechanical `Standard` → `RowMajor` update in expected compiler output.
diskann-quantization/tests/compile-fail/multi/matref_rows.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matref_get_row.stderr	Mechanical `Standard` → `RowMajor` update in expected compiler output.
diskann-quantization/tests/compile-fail/multi/matref_get_row.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_rows.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_rows_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_reborrow.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_reborrow_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_invariant.stderr	Mechanical `Standard` → `RowMajor` update in expected compiler output.
diskann-quantization/tests/compile-fail/multi/matmut_invariant.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_get_row.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_get_row_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/matmut_as_view_borrows.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_rows.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_rows_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_reborrow.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_reborrow_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_invariant.stderr	Mechanical `Standard` → `RowMajor` update in expected compiler output.
diskann-quantization/tests/compile-fail/multi/mat_invariant.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_get_row.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_get_row_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_as_view.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/tests/compile-fail/multi/mat_as_view_mut.rs	Updates compile-fail harness to new constructor naming and `RowMajor`.
diskann-quantization/src/views.rs	Switches quantization views to `diskann_utils::matrix::{DenseData, MutDenseData}`.
diskann-quantization/src/utils.rs	Updates helper utilities to use `diskann_utils::matrix::MatrixView` (and tests).
diskann-quantization/src/test_util.rs	Switches test utilities to `diskann_utils::matrix::Matrix`.
diskann-quantization/src/spherical/quantizer.rs	Updates matrix imports/usage and adapts tests to `matrix::{self, Matrix}`.
diskann-quantization/src/spherical/iface.rs	Updates docs/tests to use `diskann_utils::matrix::{Matrix, MatrixView}`.
diskann-quantization/src/scalar/train.rs	Updates scalar training signature to consume `matrix::MatrixView`.
diskann-quantization/src/scalar/quantizer.rs	Updates docs/tests to new `matrix` module paths.
diskann-quantization/src/scalar/mod.rs	Updates module docs to use `diskann_utils::matrix::Matrix`.
diskann-quantization/src/product/train.rs	Updates product-quantizer training signatures to `matrix::MatrixView`.
diskann-quantization/src/product/tables/transposed/table.rs	Replaces `MutMatrixView` with `MatrixViewMut` and rewires table compression APIs to new types.
diskann-quantization/src/product/tables/transposed/pivots.rs	Updates tests/helpers to use `diskann_utils::matrix`.
diskann-quantization/src/product/tables/test.rs	Updates test helper trait bounds from `MutMatrixView` → `MatrixViewMut`.
diskann-quantization/src/product/tables/basic.rs	Refactors pivot storage to owned/borrowed via a `PivotTable` abstraction and fixes dimension invariant to `ncols`.
diskann-quantization/src/multi_vector/mod.rs	Switches `multi_vector` to re-export `diskann_utils::matrix` types and `RowMajor`.
diskann-quantization/src/multi_vector/distance/mod.rs	Updates docs to use `RowMajor` and `MatRef::from_repr`.
diskann-quantization/src/multi_vector/distance/max_sim.rs	Updates docs to refer to `MatRef<RowMajor<_>>`.
diskann-quantization/src/multi_vector/distance/kernels/tiled_reduce.rs	Rewires tests and constructors to `RowMajor` + `from_repr`.
diskann-quantization/src/multi_vector/distance/kernels/layouts.rs	Updates layout binding impl to `MatRef<RowMajor<T>>`.
diskann-quantization/src/multi_vector/distance/kernels/f32/mod.rs	Updates kernel signatures from `Standard` → `RowMajor`.
diskann-quantization/src/multi_vector/distance/kernels/f16.rs	Updates kernel signatures from `Standard` → `RowMajor`.
diskann-quantization/src/multi_vector/distance/kernel.rs	Updates object-safe kernel trait to accept `MatRef<RowMajor<T>>`.
diskann-quantization/src/multi_vector/distance/fallback.rs	Updates fallback kernels and docs to use `RowMajor` and `from_repr`.
diskann-quantization/src/multi_vector/distance/factory.rs	Updates arch-dispatch paths to use `RowMajor` matrix views.
diskann-quantization/src/multi_vector/block_transposed.rs	Updates block-transposed impls to use `matrix::{MatrixView, MatrixViewMut}` and adapts `NewOwned` impl shape.
diskann-quantization/src/minmax/multi/mod.rs	Updates docs/examples to `RowMajor` and `Mat::from_repr`.
diskann-quantization/src/minmax/multi/meta.rs	Updates matrix type parameters (`Standard` → `RowMajor`) and tweaks allocation strategy.
diskann-quantization/src/minmax/multi/max_sim.rs	Updates tests to construct `RowMajor` input views and use `Mat::from_repr`.
diskann-quantization/src/binary/quantizer.rs	Updates tests to import `Matrix` from `diskann_utils::matrix`.
diskann-quantization/src/algorithms/transforms/random_rotation.rs	Rewires stored transform matrix type and constructors to `diskann_utils::matrix::Matrix`.
diskann-quantization/src/algorithms/kmeans/plusplus.rs	Switches `MutMatrixView` APIs to `MatrixViewMut` and updates tests.
diskann-quantization/src/algorithms/kmeans/lloyds.rs	Switches `MutMatrixView` APIs to `MatrixViewMut` and updates imports/tests.
diskann-quantization/src/algorithms/hadamard.rs	Updates tests to use `diskann_utils::matrix::{self, Matrix, MatrixView}`.
diskann-providers/src/utils/storage_utils.rs	Updates provider utilities to use `diskann_utils::matrix::{Matrix, MatrixView}`.
diskann-providers/src/utils/file_util.rs	Updates metadata/file utils to use `diskann_utils::matrix::Matrix`.
diskann-providers/src/test_utils/search_utils.rs	Updates test utils to `diskann_utils::matrix::MatrixView`.
diskann-providers/src/storage/pq_storage.rs	Updates PQ storage and tests to `diskann_utils::matrix` view types.
diskann-providers/src/storage/index_storage.rs	Updates tests to `diskann_utils::matrix::MatrixView`.
diskann-providers/src/model/pq/views.rs	Updates PQ model view conversions to new `matrix::TryFromError`.
diskann-providers/src/model/pq/strided.rs	Updates strided PQ conversion bounds to `matrix::DenseData`.
diskann-providers/src/model/pq/pq_construction.rs	Switches mutable matrix views to `MatrixViewMut` and updates call sites.
diskann-providers/src/model/pq/fixed_chunk_pq_table.rs	Updates PQ table construction to use `Matrix::try_from` and rewires view return types.
diskann-providers/src/model/pq/debug.rs	Updates PQ comparison utilities to `matrix::MatrixView` types.
diskann-providers/src/model/graph/provider/determinant_diversity.rs	Updates determinant-diversity to accept `MatrixViewMut` candidates.
diskann-providers/src/model/graph/provider/async_/inmem/spherical.rs	Updates tests and dataset construction to `diskann_utils::matrix`.
diskann-providers/src/model/graph/provider/async_/inmem/scalar.rs	Updates scalar in-mem tests to `matrix::MatrixView`.
diskann-providers/src/model/graph/provider/async_/inmem/full_precision.rs	Updates provider implementation to use `diskann_utils::matrix::Matrix`.
diskann-providers/src/index/diskann_async.rs	Updates training/test helpers to use `diskann_utils::matrix` and rewires signatures.
diskann-garnet/src/quantization.rs	Updates Garnet quantization interfaces/tests to `diskann_utils::matrix`.
diskann-garnet/src/provider.rs	Updates provider matrix imports and a `MatrixView::try_from` call site to new slice reference forms.
diskann-disk/src/storage/quant/pq/pq_generation.rs	Switches compressor interfaces from `MatrixBase` to `MatrixView/MatrixViewMut`.
diskann-disk/src/storage/quant/pq/pq_dataset.rs	Updates PQ dataset storage to use `diskann_utils::matrix::Matrix`.
diskann-disk/src/storage/quant/generator.rs	Updates generator logic to use `matrix::MatrixView/MatrixViewMut`.
diskann-disk/src/storage/quant/compressor.rs	Updates compressor trait to accept `MatrixViewMut<u8>`.
diskann-disk/src/search/provider/disk_provider.rs	Updates disk provider to import `Matrix` from `diskann_utils::matrix`.
diskann-disk/src/search/pq/quantizer_preprocess.rs	Updates scratch view construction to `matrix::MatrixViewMut`.
diskann-disk/src/build/builder/quantizer.rs	Updates build-time quantizer training input type to `matrix::MatrixView`.
diskann-bftree/src/quant.rs	Updates quantizer test setup to `diskann_utils::matrix::{Init, Matrix}`.
diskann-bftree/src/provider.rs	Updates provider docs/tests to `diskann_utils::matrix` types.
diskann-benchmark/src/utils/datafiles.rs	Updates benchmark dataset loading and conversions to `diskann_utils::matrix`.
diskann-benchmark/src/multi_vector/driver.rs	Updates multi-vector benchmark types to `RowMajor` (but still uses `Mat::from_fn`).
diskann-benchmark/src/index/streaming/managed.rs	Updates streaming managed index benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/streaming/full_precision.rs	Updates full-precision streaming benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/inmem/spherical.rs	Updates in-mem spherical benchmark to `matrix::Matrix`.
diskann-benchmark/src/index/inmem/scalar.rs	Updates in-mem scalar benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/inmem/product.rs	Updates in-mem product benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/build.rs	Updates index build benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/bftree/spherical.rs	Updates bftree spherical benchmark to `matrix::Matrix`.
diskann-benchmark/src/index/bftree/spherical_streaming.rs	Updates bftree spherical streaming benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/bftree/full_precision_streaming.rs	Updates bftree full-precision streaming benchmark to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/index/benchmarks.rs	Updates benchmark harness to `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/flat/search.rs	Updates flat search benchmark to `matrix::Matrix`.
diskann-benchmark/src/exhaustive/spherical.rs	Updates exhaustive spherical store to `matrix::Matrix` and input to `matrix::MatrixView`.
diskann-benchmark/src/exhaustive/product.rs	Updates exhaustive product store to `matrix::Matrix` and input to `matrix::MatrixView`.
diskann-benchmark/src/exhaustive/minmax.rs	Updates exhaustive minmax store to `matrix::Matrix` and input to `matrix::MatrixView`.
diskann-benchmark/src/exhaustive/algos.rs	Updates exhaustive algorithms to use `matrix::{Matrix, MatrixView}`.
diskann-benchmark/src/disk_index/search.rs	Updates disk-index benchmark search to `matrix::Matrix`.
diskann-benchmark-simd/src/lib.rs	Updates SIMD benchmark library to use `matrix::{Matrix, MatrixView}` and `matrix::Init`.
diskann-benchmark-core/src/streaming/executors/bigann/withdata.rs	Updates BigANN streaming executor to `matrix::{Matrix, MatrixView}`.
diskann-benchmark-core/src/search/ids.rs	Updates ID utilities to `matrix::{self, Matrix}` and `matrix::Init`.
diskann-benchmark-core/src/search/graph/range.rs	Updates range search to `matrix::Matrix`.
diskann-benchmark-core/src/search/graph/multihop.rs	Updates multihop search to `matrix::Matrix`.
diskann-benchmark-core/src/search/graph/knn.rs	Updates KNN search to `matrix::Matrix`.
diskann-benchmark-core/src/search/graph/inline.rs	Updates inline search to `matrix::Matrix`.
diskann-benchmark-core/src/search/api.rs	Updates search API internals to `matrix::{self, Matrix}` and `matrix::Init`.
diskann-benchmark-core/src/recall.rs	Updates recall computation/tests to `matrix::{Matrix, MatrixView}` and `matrix::Init`.
diskann-benchmark-core/src/build/graph/single.rs	Updates single build path to `matrix::Matrix`.
diskann-benchmark-core/src/build/graph/multi.rs	Updates multi build path to `matrix::Matrix`.

Comments suppressed due to low confidence (7)

diskann-utils/src/matrix.rs:753

Mat::from_fn(...) is still used in the repo (e.g. diskann-benchmark/src/multi_vector/driver.rs) but diskann_utils::matrix::Mat<RowMajor<T>> no longer defines this constructor, so the workspace won't compile. Reintroduce a dense-only from_fn that takes an already-validated RowMajor<T> and fills elements from a closure (similar to the previous API).
diskann-utils/src/matrix.rs:1226
MatrixView::row computes i * ncols without first asserting i < nrows. In release builds usize multiplication can wrap, so a very large i can return an in-bounds slice from the wrong row instead of panicking as documented. Add an explicit bounds check before doing the offset math.
diskann-utils/src/matrix.rs:1231
MatrixView::row_iter uses chunks_exact(self.ncols()), which panics when ncols == 0. Zero-column matrices are explicitly allowed by RowMajor::new, so iterating rows should remain well-defined (yielding nrows empty slices). Using the existing rows() iterator avoids the chunks_exact(0) panic.
diskann-utils/src/matrix.rs:1243
MatrixView::window_iter will panic or misbehave when ncols == 0 (division by zero in d.len() / ncols, and chunks(0) panics). It also risks ncols * batchsize overflow in release builds. Iterating by row-index windows and using subview avoids these edge cases and preserves the intended semantics.
diskann-utils/src/matrix.rs:1273
MatrixView::par_window_iter has the same ncols == 0 hazards as window_iter (par_chunks(0) panics, and d.len() / ncols divides by zero), plus potential ncols * batchsize overflow. Consider switching this to an index-window parallel iterator (using step_by) and subview, mirroring the sequential implementation.
diskann-utils/src/matrix.rs:1609
The PartialEq impls only compare ncols and the flattened slice. For ncols == 0, all matrices/views have an empty backing slice regardless of nrows, so shapes like 5x0 and 10x0 will compare equal. Include nrows (or compare repr) to make equality shape-correct for zero-column matrices.
diskann-utils/src/matrix.rs:36
The rayon::iter::ParallelIterator import is currently unused in this module (all public APIs reference rayon::iter::IndexedParallelIterator by fully-qualified path, and method bodies import the needed slice traits locally). With -D warnings in CI, this unused import will fail builds when the rayon feature is enabled. Either remove it, or import it as _ if you only need it for trait resolution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov-commenter · 2026-07-21T22:54:23Z

Codecov Report

❌ Patch coverage is 96.18321% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.31%. Comparing base (bea135b) to head (3a4a0cb).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...-quantization/src/multi_vector/distance/factory.rs	75.00%	4 Missing ⚠️
...-quantization/src/multi_vector/block_transposed.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1269      +/-   ##
==========================================
+ Coverage   91.46%   92.31%   +0.84%     
==========================================
  Files         516      515       -1     
  Lines       98200    97542     -658     
==========================================
+ Hits        89820    90042     +222     
+ Misses       8380     7500     -880

Flag	Coverage Δ
miri	`92.31% <96.18%> (+0.84%)`	⬆️
unittests	`92.27% <96.18%> (+1.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
diskann-disk/src/search/pq/quantizer_preprocess.rs	`97.22% <100.00%> (ø)`
diskann-disk/src/storage/quant/generator.rs	`98.32% <100.00%> (ø)`
diskann-disk/src/storage/quant/pq/pq_generation.rs	`92.66% <100.00%> (ø)`
diskann-garnet/src/provider.rs	`81.70% <ø> (+0.29%)`	⬆️
.../src/model/graph/provider/determinant_diversity.rs	`99.24% <100.00%> (ø)`
...ann-providers/src/model/pq/fixed_chunk_pq_table.rs	`92.83% <100.00%> (ø)`
diskann-providers/src/model/pq/pq_construction.rs	`95.88% <100.00%> (ø)`
diskann-providers/src/model/pq/views.rs	`100.00% <100.00%> (ø)`
diskann-providers/src/storage/pq_storage.rs	`87.05% <100.00%> (ø)`
...skann-quantization/src/algorithms/kmeans/lloyds.rs	`99.23% <100.00%> (ø)`
... and 20 more

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hildebrandmw · 2026-07-22T01:59:40Z

+/// [`MatrixView`] so a [`BasicTableBase`] reads its dimensions and rows from one place.
+pub trait PivotTable {
+    fn view(&self) -> MatrixView<'_, f32>;
+}


I'm not sure how I feel about this. I'll need to think about it more ...

Do you have any better suggestion?

arkrishn94

Thanks for taking this on Suryansh. I left some minor comments re errors.

Apart from that, I notice there is quite a large increase in the number of constructors and accessors for Mat and its friends. I recognize that most of this is needed because of the switch from the old view type but can you -

Do an audit and confirm that each of these accessors/constructors in matrix.rs is needed and can't be consolidated.
That these are also tested correctly? I don't notice any extra testing there.

Copilot

Pull request overview

Copilot reviewed 76 out of 78 changed files in this pull request and generated 1 comment.

arkrishn94 · 2026-07-23T18:58:56Z

+    }
+
+    /// Return a mutable base pointer for the [`Mat`].
+    pub fn as_raw_mut_ptr(&mut self) -> *mut u8 {


Should this be public?

It was pub(crate) on main. It's public now only because the framework moved into diskann-utils while the layouts that build on it (block_transposed and friends) stayed in diskann-quantization — their Repr impls need as_raw_mut_ptr to construct views over their own storage, and pub(crate) doesn't reach across the crate boundary. Same reason from_raw_parts and get_row_unchecked are public now.

If you'd still rather it not be public, do you have a preferred approach? Happy to explore alternatives.

Copilot

Pull request overview

Copilot reviewed 77 out of 79 changed files in this pull request and generated 2 comments.

+    pub(super) fn new(run: &Run) -> Self {
        let mut rng = StdRng::seed_from_u64(0x12345);
-        let queries = Mat::from_fn(
-            Standard::new(run.num_query_vectors.get(), run.dim.get())?,
-            || StandardUniform.sample(&mut rng),
+        let queries = Mat::new(
+            Init(|| StandardUniform.sample(&mut rng)),
+            run.num_query_vectors.get(),
+            run.dim.get(),
        );


                    let mat: Mat<MinMaxMeta<NBITS>> =
-                        Mat::new(meta, Defaulted).expect("NewOwned should succeed");
+                        Mat::from_default(meta).expect("NewOwned should succeed");


Copilot

Pull request overview

Copilot reviewed 76 out of 78 changed files in this pull request and generated no new comments.

Suppressed comments (3)

diskann-benchmark/src/multi_vector/driver.rs:93

Data::new used to be fallible; it now constructs Mat<RowMajor<T>> via Mat::new(...), which panics on invalid dimensions/overflow (see diskann_utils::views implementation). Since benchmark inputs come from external config, a panic here turns bad input into a hard crash instead of a normal anyhow::Result failure.

Consider restoring a fallible constructor (e.g. return Result<Self, _> and propagate RowMajor::new(...) overflow errors), updating the call sites to ? accordingly.

impl<T: Copy> Data<T>
where
    StandardUniform: Distribution<T>,
{
    pub(super) fn new(run: &Run) -> Self {
        let mut rng = StdRng::seed_from_u64(0x12345);
        let queries = Mat::new(
            Init(|| StandardUniform.sample(&mut rng)),
            run.num_query_vectors.get(),
            run.dim.get(),
        );
        let docs = Mat::new(
            Init(|| StandardUniform.sample(&mut rng)),
            run.num_doc_vectors.get(),
            run.dim.get(),
        );
        Self { queries, docs }
    }

diskann-quantization/src/utils.rs:44

The intra-doc link points at crate::multi_vector::views::Mat, but views is only re-exported as pub(crate) from multi_vector, so this link is brittle and may break in public rustdoc. Link to the public re-export (crate::multi_vector::Mat) instead.
diskann-utils/src/lib.rs:20
The PR description/review guide refers to diskann-utils/src/matrix.rs and the public path diskann_utils::matrix::*, but this crate still exposes the module as views and there is no matrix module. This mismatch makes the PR harder to review and can confuse downstream users following the description. Either update the PR description/review notes, or add a matrix module alias that re-exports views to match the stated API path.

Copilot

Pull request overview

Copilot reviewed 77 out of 79 changed files in this pull request and generated no new comments.

Suppressed comments (1)

diskann-quantization/src/minmax/multi/meta.rs:384

The expect message still says "NewOwned should succeed", but this path now uses Mat::from_default(...) / NewDefault. Updating the message will keep failures understandable and consistent with the new constructor API.

                        Mat::from_default(meta).expect("NewOwned should succeed");

suri-kumkaran added 4 commits July 21, 2026 16:30

Remove T:Copy bound from diskann-quantization\multi_vector\Mat<Standa…

671e4bf

…rd<T>> type

Main merge

c6abb1e

Improve module structure

4406457

Remove overly re-exports

0d769cf

suri-kumkaran requested review from a team and a lite review from Copilot July 21, 2026 22:36

Copilot started reviewing on behalf of suri-kumkaran July 21, 2026 22:37 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

hildebrandmw reviewed Jul 22, 2026

View reviewed changes

arkrishn94 reviewed Jul 22, 2026

View reviewed changes

Comment thread diskann-utils/src/views.rs

Comment thread diskann-utils/src/views.rs

Comment thread diskann-utils/src/matrix.rs Outdated

Address review comments

051ce98

Copilot AI review requested due to automatic review settings July 22, 2026 23:00

Copilot started reviewing on behalf of suri-kumkaran July 22, 2026 23:01 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

Comment thread diskann-quantization/src/minmax/multi/mod.rs Outdated

arkrishn94 reviewed Jul 23, 2026

View reviewed changes

suri-kumkaran added 2 commits July 24, 2026 20:51

Merge branch 'main' into users/suryangupta/unify_matrix_types

c0eb538

Minor review comments fix

16a45ef

Copilot AI review requested due to automatic review settings July 24, 2026 23:15

Copilot started reviewing on behalf of suri-kumkaran July 24, 2026 23:16 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

suri-kumkaran requested review from arkrishn94 and hildebrandmw July 27, 2026 19:44

Merge branch 'main' into users/suryangupta/unify_matrix_types

83b1594

Copilot AI review requested due to automatic review settings August 4, 2026 09:26

Copilot started reviewing on behalf of suri-kumkaran August 4, 2026 09:27 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Fix minor type name typo

3a4a0cb

Copilot AI review requested due to automatic review settings August 4, 2026 10:11

Copilot started reviewing on behalf of suri-kumkaran August 4, 2026 10:12 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Uh oh!

Conversation

suri-kumkaran commented Jul 21, 2026

How to review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov-commenter commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hildebrandmw Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

suri-kumkaran Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arkrishn94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

arkrishn94 Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

suri-kumkaran Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jul 21, 2026 •

edited

Loading