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.

Sign up for GitHub

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

Jump to bottom

create correct Singular coeffs with create_ring_from_singular_ring #773

Merged
merged 13 commits into from
Feb 23, 2024
Merged

create correct Singular coeffs with create_ring_from_singular_ring #773

merged 13 commits into from
Feb 23, 2024

Conversation

hannes14
Copy link
Member

@hannes14 hannes14 commented Feb 5, 2024

should help with oscar-system/Oscar.jl#3088

@fingolfin
Copy link
Member

Would it be possible to add a minimal test case?

@fingolfin
Copy link
Member

ping @hannes14

fingolfin
fingolfin reviewed Feb 15, 2024
View reviewed changes
src/caller.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

Thanks for adding the test. The OscarCI tests fail with a genuine error:

primary decomposition over AbsNonSimpleNumField: Error During Test at /home/runner/work/Singular.jl/Singular.jl/oscar-dev/Oscar/test/Rings/mpoly.jl:503
  Got exception outside of a @test
  TypeError: in typeassert, expected AbsNonSimpleNumField, got a value of type Singular.N_Field{AbsNonSimpleNumFieldElem}
  Stacktrace:
    [1] base_ring(R::Singular.PolyRing{AbsNonSimpleNumFieldElem})
      @ Singular ~/work/Singular.jl/Singular.jl/src/poly/poly.jl:33
    [2] (::Singular.PolyRing{AbsNonSimpleNumFieldElem})(n::Int64)
      @ Singular ~/work/Singular.jl/Singular.jl/src/poly/poly.jl:1510
    [3] _groebner_basis(r::MPolyQuoRing{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}})
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/MPolyQuo.jl:37
    [4] singular_origin_groebner_basis(Q::MPolyQuoRing{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}})
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/MPolyQuo.jl:72
    [5] singular_origin_ring
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/MPolyQuo.jl:76 [inlined]
    [6] _simplify(#unused#::Oscar.HasSingularGroebnerAlgorithm, f::MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}})
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/MPolyQuo.jl:863
    [7] simplify
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/MPolyQuo.jl:856 [inlined]
    [8] iszero
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/MPolyQuo.jl:648 [inlined]
    [9] normalise
      @ ~/.julia/packages/AbstractAlgebra/cwAAG/src/generic/Poly.jl:52 [inlined]
   [10] Poly
      @ ~/.julia/packages/AbstractAlgebra/cwAAG/src/generic/GenericTypes.jl:282 [inlined]
   [11] (::AbstractAlgebra.Generic.PolyRing{MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}})(b::Vector{MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}})
      @ AbstractAlgebra.Generic ~/.julia/packages/AbstractAlgebra/cwAAG/src/generic/Poly.jl:372
   [12] gen(R::AbstractAlgebra.Generic.PolyRing{MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}})
      @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/cwAAG/src/Poly.jl:201
   [13] #polynomial_ring#126
      @ ~/.julia/packages/AbstractAlgebra/cwAAG/src/NCPoly.jl:741 [inlined]
   [14] _change_poly_ring(R::MPolyQuoRing{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}, Rx::AbstractAlgebra.Generic.PolyRing{AbsNonSimpleNumFieldElem}, cached::Bool)
      @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/cwAAG/src/Poly.jl:3138
   [15] _make_parent(g::Hecke.var"#322#323"{MPolyQuoRing{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}, Hecke.MapDataFromNfAbsNS{Vector{MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}}}}, p::AbstractAlgebra.Generic.Poly{AbsNonSimpleNumFieldElem}, cached::Bool)
      @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/cwAAG/src/Poly.jl:3176
   [16] map_data_given_base_field_data(K::Hecke.RelSimpleNumField{AbsNonSimpleNumFieldElem}, L::MPolyQuoRing{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}, z::Hecke.MapDataFromNfAbsNS{Vector{MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}}}, y::MPolyQuoRingElem{AbstractAlgebra.Generic.MPoly{AbsNonSimpleNumFieldElem}}; check::Bool)
      @ Hecke ~/.julia/packages/Hecke/wehok/src/Map/NumField.jl:364
   [17] #map_data#325
      @ ~/.julia/packages/Hecke/wehok/src/Map/NumField.jl:399 [inlined]
   [18] #hom#316
      @ ~/.julia/packages/Hecke/wehok/src/Map/NumField.jl:151 [inlined]
   [19] hom
      @ ~/.julia/packages/Hecke/wehok/src/Map/NumField.jl:142 [inlined]
   [20] _expand_coefficient_field(R::AbstractAlgebra.Generic.MPolyRing{Hecke.RelSimpleNumFieldElem{AbsNonSimpleNumFieldElem}}; rec_depth::Int64)
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/primary_decomposition_helpers.jl:38
   [21] #_expand_coefficient_field_to_QQ#1182
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/primary_decomposition_helpers.jl:114 [inlined]
   [22] _expand_coefficient_field_to_QQ
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/primary_decomposition_helpers.jl:114 [inlined]
   [23] primary_decomposition(I::MPolyIdeal{AbstractAlgebra.Generic.MPoly{Hecke.RelSimpleNumFieldElem{AbsNonSimpleNumFieldElem}}}; algorithm::Symbol, cache::Bool)
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/mpoly-ideals.jl:725
   [24] primary_decomposition(I::MPolyIdeal{AbstractAlgebra.Generic.MPoly{Hecke.RelSimpleNumFieldElem{AbsNonSimpleNumFieldElem}}})
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/Rings/mpoly-ideals.jl:721
   [25] macro expansion
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/test/Rings/mpoly.jl:510 [inlined]
   [26] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inlined]
   [27] top-level scope
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/test/Rings/mpoly.jl:504
   [28] include
      @ ./Base.jl:385 [inlined]
   [29] macro expansion
      @ ./timing.jl:368 [inlined]
   [30] _timed_include(str::String, mod::Module; use_ctime::Bool)
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/utils/tests.jl:5
   [31] (::Oscar.var"#34#35"{Bool, Bool, Bool, Vector{Any}})()
      @ Oscar ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/utils/tests.jl:175
   [32] with_unicode(f::Oscar.var"#34#35"{Bool, Bool, Bool, Vector{Any}}, allowed::Bool)
      @ AbstractAlgebra.PrettyPrinting ~/.julia/packages/AbstractAlgebra/cwAAG/src/PrettyPrinting.jl:1707
   [33] #test_module#33
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/src/utils/tests.jl:123 [inlined]
   [34] #29
      @ ~/work/Singular.jl/Singular.jl/oscar-dev/Oscar/test/runtests.jl:137 [inlined]
   [35] (::Base.var"#834#839"{var"#29#30"})(r::Base.RefValue{Any}, args::Tuple{String})
      @ Base ./asyncmap.jl:100
   [36] macro expansion
      @ ./asyncmap.jl:234 [inlined]
   [37] (::Base.var"#850#851"{Base.var"#834#839"{var"#29#30"}, Channel{Any}, Nothing})()
      @ Base ./task.jl:417
Test Summary:                                   | Error  Total

@hannes14 could you have a look what's going on there?

@fingolfin
Copy link
Member

OK the error can be reproduce here, too, if one changes the final line of the new test to

@test base_ring(R) == base_ring(S)

which is arguably what we should use anyway.

fingolfin
fingolfin reviewed Feb 22, 2024
View reviewed changes
src/poly/poly.jl Outdated
Comment on lines 34 to 35
base_ring(R::PolyRing{N_Field{T}}) where T <: Nemo.RingElem = R.base_ring::parent_type(T)
base_ring(R::PolyRing{N_Ring{T}}) where T <: Nemo.RingElem = R.base_ring::parent_type(T)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just (unsuccessfully) papering over the actual issue

@fingolfin
Copy link
Member

I've pushed a change which should fix the immediate issue, I think, but let's see what CI reports.

@fingolfin
Copy link
Member

I have another potential simplification of the code: use basering = CoefficientRing(R) ! (which also deals with rings whose type is immutable). But I'd rather first let the CI of this version run to completion before trying yet another variant.

@fingolfin fingolfin mentioned this pull request Feb 22, 2024
Merged
fingolfin
fingolfin reviewed Feb 23, 2024
View reviewed changes
src/caller.jl
cf = libSingular.nCopyCoeff(c)
data_ptr = libSingular.nGetCoeffData(cf)
R = unsafe_pointer_to_objref(data_ptr)
basering = CoefficientRing(R) # FIXME: should we set cache=false ?
Copy link
Member

Choose a reason for hiding this comment

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

This now uses CoefficientRing which apparently was made for just this kind of purpose... It even looks up existing wrappers

@hannes14
Copy link
Member Author

@fingolfin Passes the tests,shall be merged.

@fingolfin fingolfin merged commit 34a2556 into oscar-system:master Feb 23, 2024
12 of 14 checks passed
@fingolfin
Copy link
Member

Merged. Now libsingular_jll needs to be updated again, and after Singular.jl, and then a release... Can you take care of it, @hannes14 ?

hannes14 added a commit to hannes14/Singular.jl that referenced this pull request Mar 8, 2024
@hannes14 @fingolfin
create correct Singular coeffs with create_ring_from_singular_ring (o…
2d0a3d7 
…scar-system#773)

Co-authored-by: Max Horn <[email protected]>
@hannes14 hannes14 deleted the hs/handle_Nemo_coeffs branch March 20, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hannes14 @fingolfin