Skip to content

Fix bug in comparison function in module orderings#4379

Merged
fingolfin merged 7 commits intooscar-system:masterfrom
ederc:fix-mod-ord
Dec 3, 2024
Merged

Fix bug in comparison function in module orderings#4379
fingolfin merged 7 commits intooscar-system:masterfrom
ederc:fix-mod-ord

Conversation

@ederc
Copy link
Copy Markdown
Member

@ederc ederc commented Dec 3, 2024

This should fix #4303. (@Syz-MS, @afkafkafk13):

The problem for the above issue was not the connection between Oscar and Singular, but the wrong implementation of the comparison function for lex and invlex for modules (which was accidently also described in this wrong way in the Oscar docu).

Maybe we need to adjust some tests, we will see.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.36%. Comparing base (13a2cd0) to head (9e8650a).
Report is 205 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4379      +/-   ##
==========================================
+ Coverage   84.34%   84.36%   +0.01%     
==========================================
  Files         651      652       +1     
  Lines       86749    86955     +206     
==========================================
+ Hits        73168    73359     +191     
- Misses      13581    13596      +15     
Files with missing lines Coverage Δ
src/Modules/FreeModElem-orderings.jl 96.46% <ø> (ø)
src/Rings/orderings.jl 97.58% <100.00%> (ø)

... and 4 files with indirect coverage changes

@lgoettgens
Copy link
Copy Markdown
Member

@wdecker could you please have a look at the changes to your book chapter?

-279936*t
t^3 + 93312
-279936*t
-1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In master we get

julia> L[1]
-e[1] - 279936*t*e[2] + (t^3 + 93312)*e[3]

julia> coefficients(L[1])
coefficients iterator of -e[1] - 279936*t*e[2] + (t^3 + 93312)*e[3]

julia> collect(coefficients(L[1]))
3-element Vector{AbstractAlgebra.Generic.FracFieldElem{QQPolyRingElem}}:
 -1
 -279936*t
 t^3 + 93312

In this branch we get

julia> L[1]
-e[1] - 279936*t*e[2] + (t^3 + 93312)*e[3]

julia> coefficients(L[1])
coefficients iterator of (t^3 + 93312)*e[3] - 279936*t*e[2] - e[1]

julia> collect(coefficients(L[1]))
3-element Vector{AbstractAlgebra.Generic.FracFieldElem{QQPolyRingElem}}:
 t^3 + 93312
 -279936*t
 -1

So the iteration order changed but the result itself is fine.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fingolfin : @wdecker should have a look at this to make sure that the changed iteration order does not break any wording that might be refering to a 'first entry' or something similar. It is not about the computation result!.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I checked and there does not seem to be such a problem. Also in the book there will anyway be the old output, won't it?

@fingolfin fingolfin merged commit d1cb391 into oscar-system:master Dec 3, 2024
@thofma thofma added topic: commutative algebra release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Feb 28, 2025
@thofma thofma changed the title Fix comparison function in module orderings Fix bug in comparison function in module orderings Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: commutative algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gröbner basis for modules

7 participants