Fix fmpz_mat_snf hanging on non-square and singular matrices#2596
Merged
fredrik-johansson merged 5 commits intoflintlib:mainfrom Mar 9, 2026
Merged
Fix fmpz_mat_snf hanging on non-square and singular matrices#2596fredrik-johansson merged 5 commits intoflintlib:mainfrom
fredrik-johansson merged 5 commits intoflintlib:mainfrom
Conversation
For non-square matrices and square matrices with zero determinant, fmpz_mat_snf previously used the Kannan-Bachem algorithm which suffers from intermediate coefficient explosion, causing it to hang on certain inputs (e.g. the 27x25 matrix from issue flintlib#2592). The fix computes HNF first (which is fast), extracts the nonzero rows (to avoid Iliopoulos returning gcd(0, mod) = mod for zero invariant factors), and applies the modular Iliopoulos algorithm using the product of HNF pivot entries as the modulus. Add comprehensive tests for fmpz_mat_snf including 22 hardcoded cases verified against Magma: the issue flintlib#2592 reproducer, tall/wide non-square matrices, square singular matrices, matrices with nontrivial invariant factors, and edge cases (zero, 1x1, single row/column). Fixes flintlib#2592
- Add fmpz_abs on modulus before calling Iliopoulos as a safeguard - Remove unused state parameter from _test_snf_case helper - Clarify comment explaining why HNF pivot product is a valid modulus
The previous cross-check called fmpz_mat_snf (dispatcher) which routed small matrices to kannan_bachem, so it was comparing KB vs KB. Now call both implementations directly on small square nonsingular matrices.
- Test aliasing fmpz_mat_snf(A, A) in the main randomized loop - Verify SNF rank matches fmpz_mat_rank(A) in all randomized loops - Add explicit edge cases for 0x0, 0xn, mx0 matrices - Add dedicated loop for square nonsingular matrices above cutoff (exercises the det + Iliopoulos dispatcher path) - Add hardcoded square nonsingular cases with nontrivial invariant factors (10x10 and 12x12, verified with Magma)
Collaborator
|
Fantastic! Thanks! |
|
@edgarcosta I have an undergraduate student who already has working code for SNF via iterated HNF and is just working on making it better. I am hoping he will be submitting this to FLINT soon. |
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
3 participants
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.
For non-square matrices and square matrices with zero determinant, fmpz_mat_snf previously used the Kannan-Bachem algorithm which suffers from intermediate coefficient explosion, causing it to hang on certain inputs (e.g. the 27x25 matrix from issue #2592).
The fix computes HNF first (which is fast), extracts the nonzero rows (to avoid Iliopoulos returning gcd(0, mod) = mod for zero invariant factors), and applies the modular Iliopoulos algorithm using the product of HNF pivot entries as the modulus.
Add comprehensive tests for fmpz_mat_snf including 22 hardcoded cases verified against Magma: the issue #2592 reproducer, tall/wide non-square matrices, square singular matrices, matrices with nontrivial invariant factors, and edge cases (zero, 1x1, single row/column).
Fixes #2592
At some point I might implement SNF via iterative HNF, but I wanted to get this in first