Skip to content

test(stage-1): fill untested modules + revive serde tests (#392) - #405

Merged
Mec-iS merged 2 commits into
developmentfrom
stage-1-fill-untested
Aug 9, 2026
Merged

test(stage-1): fill untested modules + revive serde tests (#392)#405
Mec-iS merged 2 commits into
developmentfrom
stage-1-fill-untested

Conversation

@Mec-iS

@Mec-iS Mec-iS commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Stage 1 of the staged test-coverage plan (#391), tracked in #392. Fills the untested substantive files and revives the disabled serde round-trip tests.

Changes

New tests for previously-untested files

  • linalg/traits/high_order.rs: implemented the /* TODO: Add tests */ module — all 4 ab() transpose-flag branches, non-square inputs, and a matmul/transpose equivalence check. (7 tests)
  • linear/lasso_optimizer.rs: direct tests for InteriorPointOptimizernew() builds ata with the correct shape and values; optimize with lambda → 0 recovers the least-squares solution on a known system. (2 tests, previously only exercised transitively via lasso.rs.)
  • error/mod.rs: tests for all 6 Failed constructors, all 8 FailedError variants, both Display impls, both PartialEq impls, and the std::error::Error trait impl. (10 tests)
  • rand_custom.rs: seeded-RNG determinism, distinct seeds diverge, and None-seed returns a usable RNG. (3 tests)

Revived 6 commented-out serde round-trip tests

Migrated serde_jsonpostcard (the serialization backend since #390 replaced bincode) for:

  • LinearRegression, RidgeRegression, Lasso, ElasticNet (src/linear/)
  • PCA, SVD (src/decomposition/)

All 6 pass — the // TODO: implement serialization for new DenseMatrix blockers are no longer relevant.

Renamed two copy-paste-misnamed tests

  • dataset::diabetes::boston_datasetdiabetes_dataset
  • algorithm::sort::quick_sort::with_capacityquick_argsort

Verification (run, not guessed)

Gate Result
cargo fmt --all -- --check exit 0
cargo clippy --all-features -- -Drust-2018-idioms -Dwarnings exit 0, no warnings
cargo test --all-features exit 0 — 473 unit (was 444, +29), 68 doctests
serde revival (cargo test --all-features --lib serde) 23 serde tests pass (6 revived + 17 pre-existing)

Each batch was verified by running before moving to the next; the optimize_with_fit_intercept known-answer test was dropped after running proved the optimizer's intercept path doesn't yield the naive slope weights.

Version bumped 0.6.00.6.1; CHANGELOG updated under [0.6.1].

Checklist

Closes #392.

Stage 1 of #391. +29 tests (444 -> 473 unit), all gates green.

New tests for previously-untested substantive files:
- linalg/traits/high_order.rs: implement the TODO mod tests covering all
  4 ab() transpose-flag branches, non-square inputs, and matmul/transpose
  equivalence.
- linear/lasso_optimizer.rs: direct tests for InteriorPointOptimizer
  (new() builds ata with correct shape; lambda->0 recovers least squares).
- error/mod.rs: tests for all 6 Failed constructors, all 8 FailedError
  variants, both Display impls, both PartialEq impls, Error trait impl.
- rand_custom.rs: seeded-RNG determinism and None-seed usability.

Revived 6 commented-out serde round-trip tests (migrated serde_json ->
postcard, the post-#390 backend) for LinearRegression, RidgeRegression,
Lasso, ElasticNet, PCA, SVD.

Renamed two copy-paste-misnamed tests:
- dataset::diabetes::boston_dataset -> diabetes_dataset
- algorithm::sort::quick_sort::with_capacity -> quick_argsort

Bump version 0.6.0 -> 0.6.1.
@Mec-iS Mec-iS added enhancement New feature or request rust Pull requests that update rust code labels Aug 9, 2026
@Mec-iS

Mec-iS commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Overall Assessment

PR #405 is clean, well-scoped, and delivers exactly what Stage 1 / #392 promised: +29 unit tests (444→473), 0 test regressions, all gates green . The commit message is precise and the verification table is honest — including the deliberately-dropped optimize_with_fit_intercept case. This is ready to merge with a few minor notes.


File-by-file Review

linalg/traits/high_order.rs — 7 new tests

Strong. The 4 transpose-flag branches (ff, ft, tf, tt) are each given known-answer fixed inputs, and the final ab_matches_direct_matmul_and_transpose test is the most valuable: it cross-validates ab() against matmul() + transpose() rather than a hand-computed expected value, so it will catch implementation drift in either direction .

One concern: the ab_true_true test comment says "(b * a)^T" but ab(true, true) computes (A^T \cdot B^T), which equals ((BA)^T) — the comment is slightly misleading since it implies b comes first in the product (it does algebraically, but the comment should say (B·A)ᵀ or better reference the identity ((AB)^T = B^T A^T) directly). Worth a one-line comment fix to avoid confusion for future contributors.

Also note: the mod tests block was previously missing #[cfg(test)] entirely — the diff adds it. That's a latent bug fix in addition to filling the TODO, since without the attribute the test code was being compiled into the release binary .


linear/lasso_optimizer.rs — 2 new tests

Strong. new_builds_ata_with_correct_shape verifies the AᵀA precomputation shape and spot-checks two diagonal values with exact hand-calculated expected values (84.0, 120.0) — this is a correctness anchor, not just a smoke test .

optimize_with_zero_lambda_recovers_least_squares is a well-designed known-answer test: using y = 2x₀ + 3x₁ exactly, checking that λ→0 recovers [2.0, 3.0] within 1e-3 . The tolerance is appropriate — tight enough to be meaningful, loose enough not to be brittle against floating-point changes in the solver.

Minor: the wasm_bindgen_test cfg-attr is present on the first test but notably the none_seed_returns_usable_rng test in rand_custom.rs uses #[cfg(not(target_arch = "wasm32"))] without the corresponding wasm test path. That's intentional and documented in the PR (unseeded path uses OS entropy, unavailable in bare wasm), but it means this test is the only one in the PR with a platform exclusion rather than a wasm alternate — worth a brief comment in the code explaining why.


error/mod.rs — 10 new tests

Excellent coverage. The exhaustive failed_error_partialeq_by_discriminant test iterates all 8 variants in an O(n²) cross-product to assert that equality holds iff i == j — this is the right way to test a discriminant-based PartialEq and will catch any future merge of two variants .

failed_implements_error_with_no_source checks both that source() returns None (verifying Failed doesn't wrap an underlying cause silently) and that &dyn Error coercion compiles — this is often overlooked but important for downstream code that does Box<dyn Error> . No issues here.


rand_custom.rs — 3 new tests

Good. The determinism test draws 8 values — long enough to make a collision vanishingly unlikely for different_seeds_produce_different_sequences . The none_seed_returns_usable_rng platform-gating is correct.

Gap: Stage 1 issue 392 also asked for "RNG determinism tests across std_rand (feature) vs SmallRng paths" . The PR covers the SmallRng default path but there's no test that compiles under --features std_rand to verify the StdRng branch is also deterministic when seeded. This is a small scope miss worth noting.


Serde revival (6 tests across linear/ and decomposition/)

Good migration. All 6 tests correctly swap serde_json::from_str(&serde_json::to_string(...))postcard::from_bytes(&postcard::to_allocvec(...)) and are gated with #[cfg(feature = "serde")] .

One subtle issue in svd.rs: the original commented-out test deserialized into SVD<f32, DenseMatrix<f32>> but the fit uses the same iris data as PCA, which produces an SVD<f64, ...>. The new test correctly changes the deserialized type to SVD<f64, DenseMatrix<f64>> . This is a silent bug fix in the revival — worth calling out explicitly in the PR description or CHANGELOG since the original commented code had a type mismatch that would have panicked.


Summary of Actionable Notes

Location Severity Note
high_order.rs::ab_true_true 🟡 Minor Comment "(b * a)^T" is algebraically correct but should be (B·A)ᵀ to match ab(a_transposed=true, b_transposed=true) semantics
high_order.rs ✅ Bonus fix Missing #[cfg(test)] on existing empty mod tests was silently compiling dead code into release — correctly fixed
rand_custom.rs::none_seed_returns_usable_rng 🟡 Minor Add inline comment explaining the #[cfg(not(wasm32))] exclusion (OS entropy unavailable)
rand_custom.rs 🟠 Scope gap std_rand feature path (StdRng) not tested — partial miss vs 392 spec
svd.rs::serde ✅ Silent fix Corrected original commented test's wrong f32 deserialization type to f64 — worth noting in CHANGELOG

The std_rand gap is the only item worth tracking before closing 392; everything else is minor polish. The PR is ready to merge as-is, with the std_rand test deferrable to a follow-up.

Sources
[1] test(stage-1): fill untested modules + revive serde tests (#392) by Mec-iS · Pull Request #405 · smartcorelib/smartcore #405

- rand_custom.rs: add std_rand_none_seed_uses_os_entropy test gated
  #[cfg(feature = "std_rand")] to cover the StdRg/OS-entropy branch
  of get_rng_impl(None) — closes the std_rand scope gap from #392.
- rand_custom.rs: add inline comment on none_seed_returns_usable_rng
  explaining the #[cfg(not(wasm32))] exclusion (OS entropy unavailable
  on bare wasm).
- high_order.rs: fix ab_true_true comment to (B·A)^T referencing the
  (AB)^T = B^T A^T identity, clarifying ab(true,true) semantics.
- CHANGELOG: note the silent SVD serde-test fix (f32 -> f64 type).
@Mec-iS

Mec-iS commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 8029a04 addressing the feedback:

  • std_rand scope gap → added std_rand_none_seed_uses_os_entropy test, gated #[cfg(feature = "std_rand")], verifying the StdRng / OS-entropy branch of get_rng_impl(None). Now 4 rand_custom tests, all pass under --features std_rand.
  • ab_true_true comment → corrected to // ab(true, true) = A^T · B^T = (B·A)^T referencing the (AB)^T = B^T A^T identity.
  • none_seed_returns_usable_rng comment → added inline explanation of the #[cfg(not(wasm32))] exclusion (OS entropy unavailable on bare wasm).
  • SVD silent fix → noted the f32f64 deserialization-type correction in the CHANGELOG.

Verified: cargo fmt --check = 0, cargo clippy --all-features -Drust-2018-idioms -Dwarnings = 0, cargo test --all-features = 0 (474 unit + 68 doctests). This closes #392 completely — all spec items now covered.

@Mec-iS
Mec-iS merged commit f7398c7 into development Aug 9, 2026
12 checks passed
@Mec-iS
Mec-iS deleted the stage-1-fill-untested branch August 9, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stage 1: Fill untested substantive files + revive disabled tests (tracking #391)

1 participant