Removing groebner_assure() in MPolyQuo

[ederc]

Removes groebner_assure() calls in for quotient rings and adds a specific accessor singular_groebner_generators() for quotient ring ideals

[codecov]

Codecov Report

Merging #2658 (7bda233) into master (c66d764) will increase coverage by 1.49%.
Report is 27 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2658      +/-   ##
==========================================
+ Coverage   71.91%   73.40%   +1.49%     
==========================================
  Files         429      441      +12     
  Lines       60812    68047    +7235     
==========================================
+ Hits        43731    49949    +6218     
- Misses      17081    18098    +1017     

Files Changed	Coverage Δ
src/Rings/groebner.jl	86.66% <ø> (+1.73%)	⬆️
src/Rings/MPolyQuo.jl	81.81% <100.00%> (-0.04%)	⬇️

... and 50 files with indirect coverage changes

[jankoboehm]

For MPolyQuoIdeal, IdealGens could provide (similar to subquotients) a data structure for relative Gröbner basis (so essentially we just add a pointer to the Gröbner basis of the modulus of the quotient ring and a show function). One thing one could then in particular do (later) is adding a dict allowing for different orderings.

[hannes14]

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

[fingolfin]

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... :-)

[wdecker]

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 ???

[jankoboehm]

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.

[fingolfin]

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....

[fingolfin]

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)

[wdecker]

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

[fingolfin]

Just talked to @hannes14 and @jankoboehm and we will merge this, I just want to first open an issue reminding us about the dim issue.