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

Removing groebner_assure() in MPolyQuo #2658

Merged
merged 3 commits into from
Aug 23, 2023
Merged

Removing groebner_assure() in MPolyQuo #2658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83efad3
adds indentation for docstring
ederc Aug 11, 2023
2e39429
remove groebner_assure() calls for quotient rings
ederc Aug 11, 2023
7bda233
Update src/Rings/groebner.jl
ederc Aug 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions src/Rings/MPolyQuo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@
end
end

function groebner_assure(r::MPolyQuoRing)
function groebner_basis(r::MPolyQuoRing)
if isdefined(r, :SQRGB)
return true
end
ordering = r.ordering
groebner_assure(r.I, ordering)
groebner_basis(r.I, ordering=ordering)
oscar_assure(r.I.gb[ordering])
SG = singular_generators(r.I.gb[ordering], ordering)
r.SQR = Singular.create_ring_from_singular_ring(Singular.libSingular.rQuotientRing(SG.ptr, base_ring(SG).ptr))
Expand Down Expand Up @@ -61,10 +61,10 @@ Base.getindex(Q::MPolyQuoRing, i::Int) = Q(base_ring(Q)[i])::elem_type(Q)
base_ring(Q::MPolyQuoRing) = base_ring(Q.I)
coefficient_ring(Q::MPolyQuoRing) = coefficient_ring(base_ring(Q))
modulus(Q::MPolyQuoRing) = Q.I
oscar_groebner_basis(Q::MPolyQuoRing) = groebner_assure(Q) && return Q.I.gb[Q.ordering].O
singular_quotient_groebner_basis(Q::MPolyQuoRing) = groebner_assure(Q) && return Q.SQRGB
singular_origin_groebner_basis(Q::MPolyQuoRing) = groebner_assure(Q) && Q.I.gb[Q.ordering].gens.S
singular_quotient_ring(Q::MPolyQuoRing) = groebner_assure(Q) && Q.SQR
oscar_groebner_basis(Q::MPolyQuoRing) = groebner_basis(Q) && return Q.I.gb[Q.ordering].O
singular_quotient_groebner_basis(Q::MPolyQuoRing) = groebner_basis(Q) && return Q.SQRGB
singular_origin_groebner_basis(Q::MPolyQuoRing) = groebner_basis(Q) && Q.I.gb[Q.ordering].gens.S
singular_quotient_ring(Q::MPolyQuoRing) = groebner_basis(Q) && Q.SQR
singular_poly_ring(Q::MPolyQuoRing; keep_ordering::Bool = false) = singular_quotient_ring(Q)
singular_poly_ring(Q::MPolyQuoRing, ordering::MonomialOrdering) = singular_quotient_ring(Q)
singular_origin_ring(Q::MPolyQuoRing) = base_ring(singular_origin_groebner_basis(Q))
Expand Down Expand Up @@ -186,13 +186,19 @@ function oscar_assure(a::MPolyQuoIdeal)
a.gens.gens.O = [r(g) for g = gens(a.gens.gens.S)]
end

function groebner_assure(a::MPolyQuoIdeal)
function groebner_basis(a::MPolyQuoIdeal)
if !isdefined(a, :gb)
a.gb = IdealGens(base_ring(a), Singular.std(singular_generators(a.gens)))
a.gb.gens.S.isGB = a.gb.isGB = true
end
end

function singular_groebner_generators(a::MPolyQuoIdeal)
groebner_basis(a)

return a.gb.S
end

@doc raw"""
gens(a::MPolyQuoIdeal)

Expand Down Expand Up @@ -638,10 +644,8 @@ true
"""
function ideal_membership(a::MPolyQuoRingElem{T}, b::MPolyQuoIdeal{T}) where T
parent(a) == base_ring(b) || error("base rings must match")
groebner_assure(b)
SR = singular_poly_ring(base_ring(b))
as = simplify(a)
return Singular.iszero(Singular.reduce(SR(as), b.gb.gens.S))
return Singular.iszero(Singular.reduce(SR(simplify(a)), singular_groebner_generators(b)))
end

Base.:in(a::MPolyQuoRingElem, b::MPolyQuoIdeal) = ideal_membership(a, b)
Expand Down Expand Up @@ -673,9 +677,8 @@ true
"""
function is_subset(a::MPolyQuoIdeal{T}, b::MPolyQuoIdeal{T}) where T
@req base_ring(a) == base_ring(b) "base rings must match"
as = simplify(a)
groebner_assure(b)
return Singular.iszero(Singular.reduce(as.gens.S, b.gb.gens.S))
simplify(a)
return Singular.iszero(Singular.reduce(singular_generators(a.gens), singular_groebner_generators(b)))
end

@doc raw"""
Expand Down Expand Up @@ -1518,8 +1521,7 @@ function dim(a::MPolyQuoIdeal)
if a.dim > -1
return a.dim
end
Copy link
Member

Choose a reason for hiding this comment

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

What is the dimension of the zero ideal?
Singular.dimension return -1 in this case, leading to a recomputaion and gb computation each time.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could use -1 to represent the zero ideal and -2 to represent "nothing was computed"?

Or add a proper "negative infinity" as already discussed elsewhere... :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Krull dimension of a proper ideal I in a ring R is by definition the Krull dimension of the quotient ring R/I. The Krull dimension of ideal(1) is not defined. In the latter case, Singular returns -1. In OSCAR, the constructor for ideals of both polynomial rings and quotient rings sets I.dim = -1. These things together cause the repeated GB computations. Therefore I suggest to follow the suggestion by Max to start with setting I.dim = -2 and change the if condition in the definition of the dim functions accordingly.

@jankoboehm @HechtiDerLachs ???

Copy link
Contributor

Choose a reason for hiding this comment

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

We should somehow store a "not comptuted". Using -1 for the zero ring and -2 for not computed sounds ok for the moment. While -1 is also used for the zero ring, the correct value for the supremum is rather -infinity since there are no chains in the zero ring. It is of course quite dangerous if someone does arithmetic with -1 and -2 directly accessing the field... What about using a type union which allows for nothing (for not computed) and eventually also -infinity.

Copy link
Member

Choose a reason for hiding this comment

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

Independent of this discussion: can this PR merged as is?

In any case, I recommend we open an issue for this discussion (so we don't forget about it), and then either merge this PR, or change this PR to draft and add a comment pointing out the PR is blocked by that issue....

Copy link
Member

Choose a reason for hiding this comment

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

There are multiple issues here:

First off, code that directly accesses this field may have a problem: indeed, but such code is bad anyway. (And we still have too much of it). I would suggest to rename the field to say _dim and then eliminate all access to it except for

  • a single dim / dimension accessor (which probably would compute a GB if there is no value set yet),
  • a function has_dim / has_dimension which returns true if the dimension is known, false otherwise
  • any function that computes the dimension (and hence needs to set _dim).

The advantage of using a name like _dim is that one grep for it and easily find bad places using it. (I am tempted to think we should do something similar for all our structs moving forward. We have far too much code directly accessing struct members everywhere).

The other issue is how to represent the values: I am happy to use nothing to represent that nothing has been computed yet. That requires to change the definition in the type from dim::Int to dim::Union{Int,Nothing}. If use right, Julia is also pretty good about still producing type stable code. E.g. this correctly deduced that the return type is always Int (of course one can also be extra cautious and change the end to return d::Int)

function dim(x)
  d = x._dim
  if d === nothing
     d = x._dim = _compute_dim(x)
  end
  return d
end

In the future we might use Union{Int,Nothing,NegInf} (I am preparing a PR right now to add NegInf to Nemo), and this should still work (Julia splits unions of up to four types by default)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@HechtiDerLachs : Does the dimension problem also appear elsewhere (local case, schemes ..)? Where do we also need to adjust the function?

groebner_assure(a)
a.dim = Singular.dimension(a.gb.S)
a.dim = Singular.dimension(singular_groebner_generators(a))
return a.dim
end

Expand Down
3 changes: 1 addition & 2 deletions src/Rings/groebner.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1484,8 +1484,7 @@ end

# modular gröbner basis techniques using Singular
@doc raw"""
groebner_basis_modular(I::MPolyIdeal{fmpq_mpoly}; ordering::MonomialOrdering = default_ordering(base_ring(I)),
certify::Bool = false)
groebner_basis_modular(I::MPolyIdeal{fmpq_mpoly}; ordering::MonomialOrdering = default_ordering(base_ring(I)), certify::Bool = false)

Compute the reduced Gröbner basis of `I` w.r.t. `ordering` using a
multi-modular strategy.
Expand Down
Loading