Make BlockTransposed cloneable#1267
Conversation
Implement cloning for the block-transposed representation by copying its complete backing allocation, including padding, and verify cloned matrices own independent storage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e50cc67e-4fa3-4f9c-ac82-89c85f4182f3
There was a problem hiding this comment.
Pull request overview
Adds Clone support for diskann_quantization::multi_vector::BlockTransposed by teaching the underlying BlockTransposedRepr how to deep-clone its entire backing allocation (including padding), enabling Mat<T>’s existing Clone machinery to produce a layout-preserving clone.
Changes:
- Implement
NewClonedforBlockTransposedReprby copyingstorage_len()elements from the raw backing allocation into a new ownedMat. - Derive
CloneforBlockTransposed, making the owned wrapper cloneable. - Add a regression test intended to validate allocation independence and clone correctness.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Write sentinels into column padding, remainder-row padding, and their intersection before cloning so the regression test distinguishes a full backing-allocation copy from logical reconstruction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e50cc67e-4fa3-4f9c-ac82-89c85f4182f3
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1267 +/- ##
==========================================
- Coverage 91.06% 90.48% -0.59%
==========================================
Files 500 498 -2
Lines 95938 95550 -388
==========================================
- Hits 87369 86455 -914
- Misses 8569 9095 +526
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Clone through BlockTransposedRef's safe slice API and convert the slice directly into a precisely sized Box allocation, avoiding both local unsafe code and Vec over-allocation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e50cc67e-4fa3-4f9c-ac82-89c85f4182f3
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e50cc67e-4fa3-4f9c-ac82-89c85f4182f3
|
@hildebrandmw and @arkrishn94,could you please help to review the pr again? Thank you! |
| impl<'a, T: Copy, const GROUP: usize, const PACK: usize> BlockTransposedRef<'a, T, GROUP, PACK> { | ||
| fn new(data: MatRef<'a, BlockTransposedRepr<T, GROUP, PACK>>) -> Self { | ||
| Self { data } | ||
| } |
There was a problem hiding this comment.
Do you mind taking a quick pass over this file to replace all instances of BlockTransposeRef { data: ... } with BlockTransposeRef::new? If we're going to have a constructor, we might as well consistently use it.
Reference Issues/PRs
Fixes #1248.
What does this implement/fix? Briefly explain your changes.
This makes
diskann_quantization::multi_vector::BlockTransposedimplementClone.The block-transposed representation now implements
NewClonedby copying its complete backing allocation, including row, column, and packing padding. This lets the existingMat<T>clone machinery provide a deep clone without reconstructing the logical matrix or changing its physical layout.A regression test verifies that the clone preserves the complete backing storage, owns a distinct allocation, and can be mutated without affecting the original.
Any other comments?
Validated with:
cargo test -p diskann-quantizationcargo clippy -p diskann-quantization --all-targets -- -D warnings