Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add iso_oscar_singular_[coeff/poly]_ring #4342

Merged
merged 1 commit into from
Dec 2, 2024
Merged

feat: add iso_oscar_singular_[coeff/poly]_ring #4342

merged 1 commit into from
Dec 2, 2024

Conversation

thofma
Copy link
Collaborator

@thofma thofma commented Nov 23, 2024

Implements the plan we agreed on #976, introducing

    iso_oscar_singular_coeff_ring
    iso_oscar_singular_poly_ring

This is similar to iso_oscar_gap, although here we must have two separate version.

This PR here applies it only as far as we need it to fix #976.

Comments/thoughts:

  • There are still many singular_poly_ring and singular_generators uses left, which can (and will with the right input) trigger ambiguity errors. But these might be fixed later.
  • @fieker this also fixes FqField if the field is not absolute.
  • This opens up the possibility to optimize the coercion for specific fields, so that Singular has a nice field to work with. For example:
    • if $K$ is an arbitrary number field, we could bring it into the form $\mathbf{Q}(\alpha)$,
    • if $R = K[e]/(e^2)$, we could turn $R[x_1,\dotsc,x_n]$ into $K[e,x_1,\dotsc,x_n]/(e^2)$

CC: @fieker @ederc

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 93.11927% with 15 lines in your changes missing coverage. Please review.

Project coverage is 84.36%. Comparing base (5076ad2) to head (3e8ef31).
Report is 36 commits behind head on master.

Files with missing lines Patch % Lines
src/Rings/oscar_singular.jl 92.82% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4342      +/-   ##
==========================================
- Coverage   84.50%   84.36%   -0.14%     
==========================================
  Files         644      652       +8     
  Lines       85594    86973    +1379     
==========================================
+ Hits        72328    73379    +1051     
- Misses      13266    13594     +328     
Files with missing lines Coverage Δ
src/Rings/mpoly.jl 73.07% <100.00%> (+0.19%) ⬆️
src/Rings/oscar_singular.jl 92.82% <92.82%> (ø)

... and 54 files with indirect coverage changes

@lgoettgens

This comment was marked as resolved.

@thofma thofma force-pushed the th/iso branch 3 times, most recently from 815f504 to de31192 Compare November 26, 2024 20:44
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks a ton for working on this, much appreciated. I have a few questions about a few bits that genuinely confuse me. Apologies if these remarks are premature or miss the point.

src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
@thofma

This comment was marked as resolved.

@afkafkafk13
Copy link
Collaborator

@thofma : slightly off-topic, but directly related to your changes

I have a question relating this to issue #4303 -- and I confess that I am not too deep into the internals of the translation:
Singular directly hardcodes a module ordering in its rings (at the moment of definition). This is not clean at all, but it is a fact. Don't we need to take care of the module ordering translation in a suitable way to avoid inconsistencies like in #4304, (which is due to the Singular default ordering for modules being used at some points)?

@thofma thofma force-pushed the th/iso branch 3 times, most recently from 1310bb7 to 365ad67 Compare November 28, 2024 19:26
@thofma
Copy link
Collaborator Author

thofma commented Nov 28, 2024

@fingolfin I think I addressed all your questions and this should be ready to go. I also refactored the code, so that singular_poly_ring/iso_oscar_poly_ring will stay in sync (in view of the changes in #4312).

@afkafkafk13 The changes here only concern how the coefficients are translated between Oscar and Singular. I don't know anything about how the modules or their interaction with Singular is designed.

@afkafkafk13
Copy link
Collaborator

@afkafkafk13 The changes here only concern how the coefficients are translated between Oscar and Singular. I don't know anything about how the modules or their interaction with Singular is designed.

Then let us get this PR merged quickly, so that this part is already in its new structure before Christian and I meet next week. I have the suspicion that this is exactly the area where we also have to take care of existing module ordering....

src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
src/Rings/oscar_singular.jl Outdated Show resolved Hide resolved
- use it in some places
@thofma
Copy link
Collaborator Author

thofma commented Nov 29, 2024

I removed all the *sense.

@thofma
Copy link
Collaborator Author

thofma commented Dec 2, 2024

@fingolfin any other concerns?

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me, merge at will

Kx = parent(defining_polynomial(F))
Fabs, QQtoFabs = Nemo._residue_field(defining_polynomial(F); absolute = true, check = false)
Fabsx, = polynomial_ring(Fabs, :x; cached = false)
return Fabs, MapFromFunc(Fabs, F, a -> F(preimage(QQtoFabs, a)), b -> begin a = Fabs(); Nemo.set!(a, b); a end)
Copy link
Member

Choose a reason for hiding this comment

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

For which types does Nemo.set!(a, b) work but not Fabs(b)?

otherwise we could do

Suggested change
return Fabs, MapFromFunc(Fabs, F, a -> F(preimage(QQtoFabs, a)), b -> begin a = Fabs(); Nemo.set!(a, b); a end)
return Fabs, MapFromFunc(Fabs, F, a -> F(preimage(QQtoFabs, a)), b -> Fabs(b))

Or even

Suggested change
return Fabs, MapFromFunc(Fabs, F, a -> F(preimage(QQtoFabs, a)), b -> begin a = Fabs(); Nemo.set!(a, b); a end)
return Fabs, MapFromFunc(Fabs, F, a -> F(preimage(QQtoFabs, a)), Fabs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not a matter of types. This is reinterpreting elements between different flint finite fields and outside the scope of "promotion".

@thofma thofma merged commit 651e595 into master Dec 2, 2024
31 checks passed
@thofma thofma deleted the th/iso branch December 2, 2024 20:31
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.

Oscar Singular coercion ambiguities
4 participants