Skip to content

fix(linalg): : scan adjacent offsets in the already-sorted vector, adding O(n) work without another allocation or sort - #500

Merged
Mec-iS merged 6 commits into
smartcorelib:mainfrom
ShryukGrandhi:fix/issue-417-no-std-step-4-replace-std-collections-ha
Oct 23, 2026
Merged

Mec-iS merged 6 commits into
smartcorelib:mainfrom
ShryukGrandhi:fix/issue-417-no-std-step-4-replace-std-collections-ha

Conversation

@ShryukGrandhi

@ShryukGrandhi ShryukGrandhi commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #417

Checklist

  • My branch is up-to-date with main branch.
  • Everything works and tested on latest stable Rust.
  • Coverage and Linting have been applied

Current behaviour

The #[cfg(debug_assertions)] block in ordered_iter_mut (src/linalg/basic/matrix.rs) uses std::collections::HashSet to check that the computed offsets are distinct. HashSet is not available under no_std + alloc, so this blocks step 4 of #413.

New expected behaviour

The debug check now uses only the Vec that the function already has:

  • each offset is checked to be in bounds (same assertion and message as before);
  • the general path already sorts the offsets into a Vec for the split_first_mut walk. A new helper, assert_distinct_offsets, scans adjacent entries of that sorted Vec and 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}) and iterator_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::collections reference remains in matrix.rs.

Tests

The old check had no direct test, so I added:

  • two should_panic tests that call ordered_iter_mut with overlapping offsets (stride smaller than ncols) 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_offsets helper (distinct offsets, and a duplicate). The helper is compiled under cfg(any(debug_assertions, test)), so these tests also run in release-mode test builds.

  • cargo test --all-features (all pass, including test_iter_mut and test_row_major)

  • cargo build --no-default-features

  • cargo fmt --all -- --check

  • cargo clippy --all-features -- -Drust-2018-idioms -Drust-2024-compatibility -Dwarnings

Change logs

Changed

  • ordered_iter_mut debug aliasing check scans the already-sorted offsets instead of using std::collections::HashSet.

Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
@ShryukGrandhi
ShryukGrandhi requested a review from Mec-iS as a code owner October 22, 2026 22:52
@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.62%. Comparing base (9eaae9e) to head (043c683).
⚠️ Report is 205 commits behind head on main.

Files with missing lines Patch % Lines
src/linalg/basic/matrix.rs 75.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mec-iS

Mec-iS commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

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 should_panic tests close a real gap. I read the diff and the surrounding ordered_iter_mut; I have not built or run it locally. I checked the new tests by hand:

  • Aliasing test (values=[0,1,2], stride 1, 2x2, row-major, axis 0): offsets are [0,1,1,2], all in bounds, and the duplicate is 1. Matches the expected message.
  • Out-of-bounds test (stride 2, 2x2): offsets are [0,1,2,3], and 3 >= len=3. Matches.

1. The extra clone and sort is avoidable (suggestion)
The general path further down already builds sorted: Vec<(usize, usize)> (offset, yield index) and sorts it with sort_unstable_by_key. The new debug block clones desired and sorts it a second time just to find duplicates. You could drop the clone and check adjacent offsets in sorted instead, e.g. sorted.windows(2).find(|w| w[0].0 == w[1].0) right after the sort. That also skips the check on the is_identity fast path, where offsets are 0..n and distinct by construction. The bounds check can stay as a cheap debug-only loop. This is a debug path, so it is not a blocker, but it removes an O(n) allocation and a redundant O(n log n) pass.

2. Consider whether the check should be debug-only at all
Without the debug block, a duplicate offset still panics, just with a less helpful message. offset - prev underflows (a panic in debug, a wrap in release), and the following split_at_mut or split_first_mut().expect("non-empty") then fails. An out-of-bounds offset ends in the same expect. If you do the change in point 1, a release-mode assert! on the adjacent-equal check costs one linear scan over data that is already sorted, and it would give a clear message in every build. This is optional and the maintainers' call.

3. Small behavioural difference (fine, worth noting)
The old code reported the first offset that repeated in iteration order. The new one reports the smallest duplicated offset, and it runs all bounds checks before any alias check. For the tests here the two agree. If more than one offset is duplicated, the reported offset can differ, which is harmless for a panic message.

4. Test plan vs. the actual no_std goal
cargo build --no-default-features still builds against std, so it proves that std::collections is gone but not that the crate is alloc-only. matrix.rs also still imports std::fmt, std::ops::Range and std::slice::Iter (all re-exported by core) and uses Box, Vec and format! (which need alloc paths under no_std). That is presumably handled by the later steps of #413. When the #![no_std] switch lands, a CI job that builds for a bare-metal target (e.g. thumbv7em-none-eabihf with --no-default-features) would make these regressions visible.

5. Nit
The doc comment on ordered_iter_mut still says debug_assert!s verify the offsets, but the code uses assert! inside a #[cfg(debug_assertions)] block. Consider rewording it to 'debug-only checks'.

Verdict: LGTM as is. Point 1 is a nice-to-have, and I'd approve with or without it.

cc @Mec-iS

ShryukGrandhi and others added 4 commits October 23, 2026 05:01
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>
@Mec-iS

Mec-iS commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Really thanks to carry on the effort for no-std, I thought it were to need a separate release but doing this module by module seems doable.

@Mec-iS

Mec-iS commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

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:

  1. Update the PR description to match the final implementation: it scans adjacent offsets in the already-sorted vector, adding O(n) work without another allocation or sort, rather than cloning and sorting offsets.
  2. Clarify release-mode coverage: the helper tests can run in release mode, but the iterator-level panic tests and the production duplicate-check call remain debug-only.
  3. Describe the diagnostics as retaining the same format rather than identical behavior for all malformed inputs: bounds checks now run before duplicate detection, and multiple duplicates are reported in sorted-offset order.

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.

@Mec-iS Mec-iS changed the title fix(linalg): drop std HashSet from ordered_iter_mut debug check fix(linalg): : scan adjacent offsets in the already-sorted vector, adding O(n) work without another allocation or sort Oct 23, 2026
@Mec-iS
Mec-iS merged commit a904b77 into smartcorelib:main Oct 23, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no-std Step 4: replace std::collections::HashSet in ordered_iter_mut debug check

2 participants