oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
312 stars 113 forks source link

Oscar Singular coercion ambiguities #976

Open thofma opened 2 years ago

thofma commented 2 years ago
julia> A, t = rational_function_field(QQ, "t");

julia> B, s = rational_function_field(A, "s");

julia> R, (x,y,z) = polynomial_ring(B, ["x", "y", "z"]);

julia> I = ideal(R, [x]); x in I
ERROR: MethodError: (::AbstractAlgebra.Generic.RationalFunctionField{AbstractAlgebra.Generic.Rat{fmpq}})(::Singular.n_FieldElem{AbstractAlgebra.Generic.Frac{AbstractAlgebra.Generic.Poly{AbstractAlgebra.Generic.Rat{fmpq}}}}) is ambiguous. Candidates:
  (b::AbstractAlgebra.Ring)(a::Union{Singular.n_FieldElem{T}, Singular.n_RingElem{T}} where T) in Oscar at /Users/thofma/.julia/dev/Oscar/src/Rings/mpoly.jl:722
  (a::AbstractAlgebra.Generic.RationalFunctionField)(b::RingElem) in AbstractAlgebra.Generic at /Users/thofma/.julia/dev/AbstractAlgebra/src/generic/RationalFunctionField.jl:567
Possible fix, define
  (::AbstractAlgebra.Generic.RationalFunctionField)(::Union{Singular.n_FieldElem{T}, Singular.n_RingElem{T}} where T)
Stacktrace:

julia is right! This is ambiguous and the fix works only for RationalFunctionField. But the problem persists for any ring type S which has a method of the form (R::S)(x::RingElem).

The problem is that https://github.com/oscar-system/Oscar.jl/blob/af90aae4743389fa07914ff3ca4717b744547e79/src/Rings/mpoly.jl#L721-L724 is too broad in the first argument (or too specific in the second).

I see two options:

  1. Do the dirty for T in subtypes(Ring); @eval trick. Problem is that this is not extensible, so only works for the ring types which have been defined till this point.
  2. Do not use R(x) to do the conversion from the Singular to the Oscar side, but maybe singular_to_oscar(R, x).

Any other ideas? @tthsqe12 @fieker?

tthsqe12 commented 2 years ago

method 2. Please relieve us of this R(x) syntax.

thofma commented 2 years ago

As we have all experienced, the conversion between Singular rings and the Oscar side can be a bit error prone. Note that the same problem was exhibited by the GAP <-> Oscar conversions, which was solved by introducing the wonderful function

function iso_oscar_gap(R::Ring)

which returns a map object with proper domain and codomain realising this isomorphism.

I propose that we do the same here and introduce

function iso_oscar_singular(R::MPolyRing)

which returns a map object that one can apply etc.

What do you think @tthsqe12? Since @ederc is also a fan of singular_ring, I am also pinging you here :)

P.S.: We might also need iso_oscar_singular_coefficient_ring, to properly fix the bug in #975.

ederc commented 2 years ago

Having discussed this with @thofma I think this might be a good and more secure concept handling conversions.

tthsqe12 commented 2 years ago

so singular_ring should be no more, and in its place stands singular_coefficient_ring (number) singular_polynomial_ring (spoly) iso_oscar_singular_coefficient_ring (goes both ways) iso_oscar_singular_polynomial_ring (" ") ?

thofma commented 2 years ago

We probably will remove singular_*_ring altogether and just provide iso_oscar_singular_coefficient_ring/iso_oscar_singular_polynomial_ring.

tthsqe12 commented 2 years ago

ok

fingolfin commented 1 year ago

What's the status of this? The original issue seems to be resolved, but I see no trace of iso_oscar_singular_* functions. So perhaps there was a different workaround applied? But perhaps we still want iso_oscar_singular_*?

thofma commented 1 year ago

Not resolved. We just fixed the cases that popped up.