saloon: relax eig symmetry/Hermitian asserts to a tolerance#53
Merged
Conversation
eig asserted EXACT symmetry (=) on the input, which crashed on two classes of
legitimately-(skew-)symmetric matrices the review found:
- a Gram matrix M^T*M straight out of Lagoon mmul, symmetric only up to ULP
because [i,j] and [j,i] accumulate in different orders;
- under %d/%u rounding, conj(a+0i) = a + (-0.0) (IEEE 0-0=-0), so the
Hermitian diagonal check failed bit-exactly on essentially any matrix.
Replace the exact check with a relative-tolerance, magnitude-based one
(+stol/+near/+cnear): accept inputs symmetric/Hermitian within ~stol, reject
genuinely asymmetric ones (entries differing by O(1)) so they still can't
silently get the wrong algorithm. Being magnitude-based, +-0.0 conjugate-pair
sign differences pass automatically.
Also surfaces the P1 caller hazards in the +eig doccord: rtol must match the
component width; the bare `sa` default rtol=0x1 is a denormal (always trips the
60-sweep cap + ~& warning) so callers must use +sake; and fixes the stray
"(skew-)symmetry" wording.
New saloon-eig-tol: near-symmetric (1 ULP) accepted -> eigenvalues {1,3};
clearly asymmetric crashes (expect-fail). 18 symmetric + 5 Hermitian
regressions still green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P0 fix (Gnome/Angel review)
eigasserted exact symmetry (=) on its input, crashing on two classes of legitimately symmetric matrices:MᵀMfrom Lagoonmmul— symmetric only up to ULP, since[i,j]and[j,i]accumulate in different orders. The obvious way to produce a symmetric matrix tripped the assert.%d/%urounding,cconjdoes0 − Imand IEEE gives0 − 0 = −0, soconj(a+0i) ≠ a+0ibit-exactly → the Hermitian diagonal check crashed on essentially any Hermitian matrix.Fix: replace exact
=with a relative-tolerance, magnitude-based check (+stol/+near/+cnear): accept inputs symmetric/Hermitian within~stol, still reject genuinely asymmetric ones (entries differing by O(1)) so they can't silently get the wrong algorithm (that's Phase B's job). Magnitude-based ⇒±0.0conjugate-pair sign differences pass automatically.Also folds in the agreed P1 doccord surfacing on
+eig:rtolmust match the component width; the bare-sadefaultrtol=0x1is a denormal that always trips the 60-sweep cap +~&warning (so call+sake); and the result is returned, not silently wrong. Fixes the stray "(skew-)symmetry" wording.Tests
saloon-eig-tol: near-symmetric (1 ULP off) accepted → eigenvalues{1,3}; clearly asymmetric ([[2,1],[2,2]]) crashes (expect-fail). The 18 symmetric + 5 Hermitian eig regressions stay green.Note: touches the saloon
%linalghelper region, so it'll conflict with the open@ch/@cqPR #50 — whichever merges second rebases.🤖 Generated with Claude Code