Skip to content

audit: validate tolerance arguments consistently across the crate #94

@acgetchell

Description

@acgetchell

Summary

Tolerance arguments (tol, rel_tol) are accepted by several public APIs but are validated
inconsistently across the crate. Some entry points debug_assert! on tol >= 0.0,
others silently accept negative / NaN tolerances, and none explicitly document or
reject +∞. This issue tracks a crate-wide audit to unify the contract for all
tolerance parameters in preparation for the v0.5.0 breaking API revision.

Current State

Tolerance-accepting public APIs today:

  • Matrix::lu(tol: f64) -> Result<Lu<D>, LaError>Lu::factor
    • No debug_assert! on tol. A negative tol silently makes the
      pivot_abs <= tol check unreachable, turning every matrix into "non-singular";
      a NaN tolerance makes the comparison always false, with the same effect.
  • Matrix::ldlt(tol: f64) -> Result<Ldlt<D>, LaError>Ldlt::factor
    • debug_assert!(tol >= 0.0, "tol must be non-negative") at src/ldlt.rs:53.
      This also rejects NaN (because NaN >= 0.0 is false), but the message
      does not mention NaN and release builds still accept it silently.
  • Matrix::is_symmetric(rel_tol: f64) / Matrix::first_asymmetry(rel_tol: f64)
    • debug_assert!(rel_tol >= 0.0, "rel_tol must be non-negative (got {rel_tol})")
      at src/matrix.rs:223. Docs claim "negative or NaN" is rejected, and the
      assert does catch NaN in debug builds, but this is not enforced in release.
  • Ldlt::solve_vec / Lu::solve_vec reuse the stored self.tol from factorization;
    validation at factor-time is the only line of defense.

DEFAULT_SINGULAR_TOL / DEFAULT_PIVOT_TOL (src/lib.rs:100-106) are both 1e-12
and are always finite and non-negative, so the default code paths are unaffected;
this only bites callers who pass custom tolerances.

Proposed Changes

  1. Define a single contract for tolerance parameters (proposal):
    • tol must be finite (tol.is_finite()) and tol >= 0.0.
    • +∞ is explicitly rejected even though the comparisons would "work".
  2. Enforce the contract uniformly across Matrix::lu, Matrix::ldlt,
    Matrix::is_symmetric, Matrix::first_asymmetry, and any future
    tolerance-accepting API:
    • Add the same debug_assert! to Lu::factor that Ldlt::factor has today.
    • Extend both to also assert finiteness (debug_assert!(tol.is_finite())).
  3. Update docs on every tolerance-accepting method to state the contract
    verbatim (negative / NaN / +∞ → debug panic, release = garbage-in-garbage-out),
    matching the wording already used in Matrix::is_symmetric.
  4. Add regression tests (gated on #[cfg(debug_assertions)], #[should_panic])
    covering negative, NaN, and +∞ tolerance for each entry point, using the
    gen_*_tests! macro pattern for D=2..=5 per the AGENTS.md dimension-coverage rule.
  5. Consider (stretch) returning a typed error (e.g. LaError::InvalidTolerance)
    in release builds instead of silent acceptance — this is a breaking change and
    is why this issue targets v0.5.0.

Benefits

  • Removes a class of silent numerical bugs where a bad tol hides singular matrices.
  • Makes the public API surface predictable: one contract, documented once.
  • Aligns with the existing finiteness-propagation guarantees already provided by
    inf_norm, lu, and ldlt for matrix entries.

Implementation Notes

  • Keep validation in debug_assert! form for now to preserve zero-cost release
    builds; a release-time LaError variant is the stretch goal and should be
    discussed separately before implementation.
  • The new assertions must land together with doc updates so the contract is
    discoverable from rustdoc.
  • Related context: docs: strengthen LDLT symmetry precondition documentation #84 (LDLT symmetry precondition docs) established the
    "debug-assert + document + provide a public predicate" pattern that this
    audit should extend to tolerances.

Related: #82, #83 (other v0.5.0 breaking-API cleanups)

Metadata

Metadata

Assignees

No one assigned

    Labels

    apienhancementNew feature or requestrustPull requests that update rust codevalidation

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions