Relax Copy to Clone on Standard<T> matrix elements#1265
Conversation
There was a problem hiding this comment.
Pull request overview
This PR relaxes Mat<Standard<T>> to support non-Copy element types by switching construction/cloning paths from T: Copy to T: Clone, while keeping Standard<T> metadata Copy (so Repr: Copy continues to hold). This aligns multi_vector::Mat behavior more closely with diskann-utils::Matrix<T> and unblocks future unification work.
Changes:
- Relax
T: Copybounds toT: Clone(or remove entirely for views) acrossStandard<T>’sRepr*/New*impls andMat/MatRef/MatMutimpl blocks. - Update owned initialization and cloning paths to use
.clone()/.cloned()instead of copying. - Add a
String-backed test to validate fill,from_fn, mutation, clone, and slice-backed views for non-Copyelements.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1265 +/- ##
==========================================
+ Coverage 90.16% 91.16% +0.99%
==========================================
Files 509 510 +1
Lines 97238 97291 +53
==========================================
+ Hits 87678 88694 +1016
+ Misses 9560 8597 -963
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hildebrandmw
left a comment
There was a problem hiding this comment.
Thanks! Just a few minor comments.
What
Mat<Standard<T>>requiredT: Copy. Now it works for anyCloneelement (and anyTfor views) — matching whatdiskann-utils::Matrix<T>already allows.Why
The
Copybound wasn't load-bearing; it came from a naive derive and a few over-broadwhereclauses. Relaxing it is a prerequisite for unifyingMatrix<T>withMat<Standard<T>>.How
Standard<T>: hand-write unconditionalCopy/Clone(it's just dims +PhantomData), drop the unusedEq, keepPartialEq.T: CopyfromRepr/ReprMut/ReprOwned/NewRef/NewMutand theMat/MatRef/MatMutimpl blocks.NewOwned/NewCloned:Copy→Clone(value.clone(),.cloned()).String-element test covering fill /from_fn/ mutation / clone / views.Repr metadata stays
Copy(required byRepr: Copy, free forPhantomData) — only the element bound relaxes.