Skip to content

Throw for leading/trailing_coefficient of zero in multivariate setting#2153

Merged
thofma merged 3 commits intoNemocas:masterfrom
lgoettgens:lg/leading_coefficient
Sep 5, 2025
Merged

Throw for leading/trailing_coefficient of zero in multivariate setting#2153
thofma merged 3 commits intoNemocas:masterfrom
lgoettgens:lg/leading_coefficient

Conversation

@lgoettgens
Copy link
Copy Markdown
Member

@lgoettgens lgoettgens commented Sep 3, 2025

This is the AA part of the discussion result of oscar-system/Oscar.jl#5231.

@wdecker @fingolfin could you please check that this aligns with what we discussed in triage yesterday? Thanks!

The test failures in Nemo are expected and need to be dealt with in the same manner as the changes to tests here.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.95%. Comparing base (2654e69) to head (47c2729).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
src/MPoly.jl 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2153      +/-   ##
==========================================
- Coverage   87.95%   87.95%   -0.01%     
==========================================
  Files         127      127              
  Lines       31735    31740       +5     
==========================================
+ Hits        27914    27918       +4     
- Misses       3821     3822       +1     

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

@lgoettgens lgoettgens marked this pull request as ready for review September 3, 2025 22:57
Copy link
Copy Markdown
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

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

Do you plan to make the adjustments also in other downstream packages (if they arise)? I don't have the bandwidth for this and I don't want us to be stuck with this next week or so.

@lgoettgens
Copy link
Copy Markdown
Member Author

Do you plan to make the adjustments also in other downstream packages (if they arise)? I don't have the bandwidth for this and I don't want us to be stuck with this next week or so.

The downstream tests for Singular (https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/17447883134/job/49546580545?pr=2153), Oscar (https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/17447883135/job/49546584478?pr=2153), and Hecke (https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/17447883140/job/49546579898?pr=2153) did not show any issues. It's just the Nemo tests that have to be adapted. And that should be in scope for me.

But please, before merging this, someone should double-check that this PR here indeed aligns with the decision from triage. (I got really confused at some point and may have changed everything in the completely wrong direction.

@lgoettgens
Copy link
Copy Markdown
Member Author

It's just the Nemo tests that have to be adapted. And that should be in scope for me.

Already done in Nemocas/Nemo.jl#2137. The downstream tests in https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/17447883154/job/49573635095 will verify that this will indeed do the job.

@thofma
Copy link
Copy Markdown
Member

thofma commented Sep 4, 2025

Thanks

@thofma thofma merged commit 9efdab4 into Nemocas:master Sep 5, 2025
21 of 27 checks passed
@lgoettgens lgoettgens deleted the lg/leading_coefficient branch September 5, 2025 18:06
thofma referenced this pull request Sep 6, 2025
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants