Repository navigation
fix(linalg): : scan adjacent offsets in the already-sorted vector, adding O(n) work without another allocation or sort - #500
Conversation
Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #500 +/- ##
===========================================
+ Coverage 43.97% 85.62% +41.64%
===========================================
Files 85 96 +11
Lines 7281 9379 +2098
===========================================
+ Hits 3202 8031 +4829
+ Misses 4079 1348 -2731 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks @ShryukGrandhi. This is a clean, minimal step for #413/#417: the logic is behaviour-preserving, the panic messages are unchanged, and the two new
1. The extra clone and sort is avoidable (suggestion) 2. Consider whether the check should be debug-only at all 3. Small behavioural difference (fine, worth noting) 4. Test plan vs. the actual no_std goal 5. Nit Verdict: LGTM as is. Point 1 is a nice-to-have, and I'd approve with or without it. cc @Mec-iS |
Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
|
Really thanks to carry on the effort for |
|
Review: no blocking correctness issues found in the revision reviewed. Reusing the existing sorted offsets for the duplicate check is sound and avoids an additional allocation or sorting pass. The identity fast path cannot contain duplicates, and the bounds checks still run before it in debug builds. The added tests cover the intended aliasing and out-of-bounds diagnostics. Non-blocking suggestions:
This is a useful incremental step toward no_std + alloc compatibility, not a complete no_std migration; broader changes should stay outside this PR. Validation: reviewed the diff and surrounding implementation and checked GitHub CI; I did not build or run the Rust tests locally. |
Fixes #417
Checklist
Current behaviour
The
#[cfg(debug_assertions)]block inordered_iter_mut(src/linalg/basic/matrix.rs) usesstd::collections::HashSetto check that the computed offsets are distinct.HashSetis not available underno_std + alloc, so this blocks step 4 of #413.New expected behaviour
The debug check now uses only the
Vecthat the function already has:Vecfor thesplit_first_mutwalk. A new helper,assert_distinct_offsets, scans adjacent entries of that sortedVecand reports equal entries as aliasing. This adds O(n) work and no extra allocation or sort. The identity fast path cannot contain duplicates, so it does not need this check.The diagnostics keep the same format (
iterator_mut: offset {o} out of bounds (len={len})anditerator_mut: aliasing detected at offset {o}). The behaviour for all malformed inputs is not identical: all bounds checks now run before the duplicate check, and when there are multiple duplicates, the first one in sorted-offset order is reported.The production checks are still debug-only. Release builds and the iterator logic do not change.
No
std::collectionsreference remains inmatrix.rs.Tests
The old check had no direct test, so I added:
two
should_panictests that callordered_iter_mutwith overlapping offsets (stride smaller thanncols) and with an out-of-bounds offset. These iterator-level tests are#[cfg(debug_assertions)], so they run only in debug builds, the same as the production checks;two tests for the
assert_distinct_offsetshelper (distinct offsets, and a duplicate). The helper is compiled undercfg(any(debug_assertions, test)), so these tests also run in release-mode test builds.cargo test --all-features(all pass, includingtest_iter_mutandtest_row_major)cargo build --no-default-featurescargo fmt --all -- --checkcargo clippy --all-features -- -Drust-2018-idioms -Drust-2024-compatibility -DwarningsChange logs
Changed
ordered_iter_mutdebug aliasing check scans the already-sorted offsets instead of usingstd::collections::HashSet.