Repository navigation
fix: take the sparse solver path only for a positive-definite K - #145
Merged
Merged
Conversation
Shift-invert at zero returns the eigenvalues smallest in magnitude, which are the lowest only when K is positive definite. With an indefinite K, such as a tower past buckling under gravity, the sparse path dropped the strongly negative modes that the dense path keeps, so the result changed with mesh refinement alone. A banded Cholesky now gates the sparse path; anything not definite goes dense. The sparse path also builds its symmetrised matrices sparsely, which more than pays for the check.
Owner
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Merged
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
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.
Reported on LinkedIn against 1.19.0 (9856574):
solve_modesreturned a different spectral window on the sparse path than on the dense one wheneverKis indefinite.Root cause
Above 500 DOF a symmetric subset solve goes through
eigsh(K, M, sigma=0, which='LM'). Shift-invert at zero returns the eigenvalues smallest in magnitude. Those are the lowest only whenKis positive definite. With a strongly negative eigenvalue present, the sparse path left it out and backfilled the window with positive modes, while the dense path, which is built to keep negative eigenvalues, returned it.K = diag(-100, 1, ..., 500),M = I, three modes gave[1, 2, 3]on the sparse path and[-100, 1, 2]on the dense path at 400 DOF.run(gravity=True)with 1000 times its RNA mass. The default 40-node mesh (dense) returns four negative eigenvalues, down to λ = −16.7. Refined to 120 nodes (sparse) it returned the near-zero pair plus two positive modes. At 60 and 300 times the RNA mass the two paths agreed, because the negative eigenvalues were still small in magnitude.Fix
solve_modestakes the sparse path only after_is_positive_definiteconfirmsKadmits a Cholesky factorisation. Anything else, indefinite or singular, goes to the dense path, which returns the algebraically lowest modes.scipy.linalg.cholesky_banded) with the band measured fromKitself. The assembled stiffness has a half-bandwidth of 10 at every mesh size tried, so the factorisation is about a millisecond at 9000 DOF. A dense Cholesky there takes about 2 s. A model with wider coupling is still checked exactly, only less cheaply.KandMfor the sparse path are now built sparsely (csc + csc.T) instead of from a dense0.5 (K + K^T)copy, and the guard shares the same sparseK. That more than pays for the check.Performance (NREL 5MW land tower, 6 modes, best of 2 to 3 runs)
Positive-definite results are unchanged within the sparse solver's own run-to-run scatter: master against this PR differs by 3e-11 relative, and two runs of the same code by 9e-11, from ARPACK's random starting vector. The bundled reference decks and samples regenerate with no content change.
Tests (
tests/fem/test_sparse_solver.py)Kwith two negative eigenvalues and a scaled diagonalM, aKwith three zero modes, and an SPD control that must still go sparse._is_positive_definiteagainst a dense Cholesky on random symmetric matrices, a tridiagonal band, singular and empty matrices, a full-width coupling and a tiny asymmetry.VALIDATION.mdrows added;scripts/audit_validation_claims.pyis clean.ruffandmypyclean.