Skip to content

Ensure det for 0x0 matrices always returns 1 (and not 0)#2288

Merged
lgoettgens merged 3 commits into
masterfrom
mh/fix-det-0x0
Feb 3, 2026
Merged

Ensure det for 0x0 matrices always returns 1 (and not 0)#2288
lgoettgens merged 3 commits into
masterfrom
mh/fix-det-0x0

Conversation

@fingolfin
Copy link
Copy Markdown
Member

Should get some tests... later

@fingolfin fingolfin added bug Something isn't working bugfix release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes bug: wrong result Bugs that returned incorrect results labels Jan 10, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 10, 2026

Codecov Report

❌ Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.68%. Comparing base (b548c14) to head (a4e789b).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
ext/TestExt/Rings-conformance-tests.jl 86.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2288      +/-   ##
==========================================
+ Coverage   88.22%   88.68%   +0.45%     
==========================================
  Files         126      126              
  Lines       32000    33859    +1859     
==========================================
+ Hits        28233    30027    +1794     
- Misses       3767     3832      +65     

☔ 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.

@thofma
Copy link
Copy Markdown
Member

thofma commented Jan 11, 2026

Later as in "I will add it to this PR later" or as in later later?

@fingolfin fingolfin removed bug Something isn't working bug: wrong result Bugs that returned incorrect results labels Jan 12, 2026
@fingolfin
Copy link
Copy Markdown
Member Author

My plan was "Later as in 'this PR'". But it is actually difficult to add a "natural" test because all the det methods actually were fine, the problem was in det_fflu which is internal (and the calls to it all catch the problematic case first), and in det_interpolation (which is not used at all!).

The only thing I could think of was to add some direct tests for these. In the end I did it by adding them conformance test suite. That is perhaps not the ideal way, athe new tests are focused more on testing det_fflu etc. than the ring interfaces. Then again, this allows us to add general check for det...

The only risk I can think of is that this might break for some rings using the conformance tests. But that's something we can deal with.

Comment thread src/Matrix.jl
function det_fflu(M::MatElem{T}) where {T <: RingElement}
n = nrows(M)
if n == 0
return base_ring(M)()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For reference: this was bad as it returned zero.

Comment thread src/Matrix.jl
error("Generic interpolation requires a domain type")
R = base_ring(M)
if n == 0
return R()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

... and this also returned zero

@fingolfin fingolfin requested a review from lgoettgens January 12, 2026 12:54
Copy link
Copy Markdown
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

Lgtm, but please wait until we have working downstream tests again

@lgoettgens lgoettgens enabled auto-merge (squash) January 21, 2026 14:46
@lgoettgens lgoettgens disabled auto-merge January 21, 2026 18:13
@lgoettgens lgoettgens added bug: wrong result Bugs that returned incorrect results and removed bugfix labels Jan 22, 2026
@lgoettgens
Copy link
Copy Markdown
Member

It looks like some det methods in Nemo also have the same bug, so introducing the tests here breaks CI there. I would suggest to first fix the bug in Nemo, and then re-run CI here.

@fingolfin fingolfin closed this Feb 2, 2026
@fingolfin fingolfin reopened this Feb 2, 2026
@fingolfin fingolfin changed the title Ensure det for 0x0 matrices return 1 Ensure det for 0x0 matrices always returns 1 (and not 0) Feb 3, 2026
@fingolfin
Copy link
Copy Markdown
Member Author

CI passes now. Shall we merge, @lgoettgens ?

@lgoettgens lgoettgens merged commit 3e14b81 into master Feb 3, 2026
43 of 45 checks passed
@lgoettgens lgoettgens deleted the mh/fix-det-0x0 branch February 3, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug: wrong result Bugs that returned incorrect results release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants