Skip to content

Conversation

@JohnAAbbott
Copy link
Collaborator

New heuristic function is_zero_det_probabilistic; incl. doc & tests.

@JohnAAbbott
Copy link
Collaborator Author

This is currently restricted to ZZMatrix. An extension to QQMatrix should be fairly easy. Extending to (univariate) polynomials over ZZ or QQ would take some more work, but not excessively much.

@codecov
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

❌ Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.31%. Comparing base (178d272) to head (bf68a1d).
⚠️ Report is 66 commits behind head on master.

Files with missing lines Patch % Lines
src/flint/fmpq_mat.jl 79.16% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2152      +/-   ##
==========================================
+ Coverage   88.20%   88.31%   +0.10%     
==========================================
  Files          99      100       +1     
  Lines       37079    37247     +168     
==========================================
+ Hits        32707    32893     +186     
+ Misses       4372     4354      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 465 to 467
catch IgnoreError
# The prime was bad, so we just ignore it
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The prime was bad, so we just ignore it" or your code triggers another error and it is not caught. This should test agains the real error and rethrow the error otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The prime was bad, so we just ignore it" or your code triggers another error and it is not caught. This should test against the real error and rethrow the error otherwise.

Modified the code; not sure what else it could throw (but maybe the coercion code will change in the future?) Really the documentation should also indicate which exceptions might be thrown...

@thofma
Copy link
Member

thofma commented Oct 13, 2025

There is a lot of code duplication. How much slower is it to just do is_zero_det_probablisitic(numerator(x)) in the rational case?

@JohnAAbbott
Copy link
Collaborator Author

There is a lot of code duplication. How much slower is it to just do is_zero_det_probablistic(numerator(x)) in the rational case?

Yes, there is duplication, but I don't know how to avoid that without risking a significant speed penalty in some cases, e.g. the current test uses a matrix 250x250 whose entries are 1/k for k ranging from 1 to 250^2; this takes less than 0.02s on my computer, but computing the result by taking the numerator takes nearly 1.8s -- roughly 100 times slower.
OK, I could convert the ZZMatrix into a QQMatrix and then use the QQMatrix function... but that seems wasteful.

@thofma
Copy link
Member

thofma commented Oct 13, 2025

Thanks for checking. I agree that the overhead of converting to ZZMatrix is too large.

Co-authored-by: Tommy Hofmann <[email protected]>
@JohnAAbbott
Copy link
Collaborator Author

Converting to a non-draft PR. The extensions mentioned in comment 2 (near the start of this discussion) can be dealt with separately if there is call for them.

@JohnAAbbott JohnAAbbott marked this pull request as ready for review October 29, 2025 14:52
# we switch to larger 61-bit primes (which are faster amortized).

@doc raw"""
is_zero_det_probabilistic(M::QQMatrix; modulus_bitsize::Int = 100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests whether "is_zero(det(M)) is probably true (if it returns true) and otherwise definitely false (if it returns false)

We have is_probable_prime so maybe this should be is_probably_zero_det (but sounds weird?).

Having it at end makes it easier to find, though.


Should this function be added to the documentation? It is currently being exported in this PR, which to me suggests it should be in the manual...

@fingolfin fingolfin requested a review from fieker January 14, 2026 10:50
@fingolfin fingolfin removed the triage label Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants