Fix #4018

[joschmitt]

(Sorry for the duplicate, #4037 was the wrong branch.)

This fixes #4018 for me.

Apparently, one should not set keep_ordering = true in this case. Otherwise the ordering on the Singular side is set to what is nowadays called internal_ordering, so lex in this case.
I don't understand what the point of keep_ordering is. Does somebody know?

[fingolfin]

Maybe @ederc or @jankoboehm know more?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.08%. Comparing base (a7e4188) to head (c38c14b).
Report is 48 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4038      +/-   ##
==========================================
+ Coverage   84.58%   85.08%   +0.49%     
==========================================
  Files         597      613      +16     
  Lines       82237    87693    +5456     
==========================================
+ Hits        69559    74610    +5051     
- Misses      12678    13083     +405     

Files with missing lines	Coverage Δ
src/Rings/mpoly-graded.jl	91.36% <100.00%> (ø)
src/Rings/mpoly-ideals.jl	93.21% <100.00%> (-0.11%)	⬇️
src/Rings/mpoly.jl	72.87% <100.00%> (+0.13%)	⬆️

... and 60 files with indirect coverage changes

[joschmitt]

I realize now that my "fix" only works by coincidence, that is, because the ordering we want on the Singular side in this example happens to be the default (degrevlex).
I will try to produce a minimal example of what I mean tomorrow.

[wdecker]

Tracing the bug here, I see a couple of things with idealGens, groebner_assure etc which I do not quite understand. I must discuss this with @ederc, @hannes14.
@joschmitt For example, making another change in the function grassmann_pluecker_ideal, the "problem disappears":

return ideal(IdealGens(base,base.(gens(I)), o;
keep_ordering=false,
isReduced=false,
isGB=true))

[joschmitt]

I did another fix now. I am more confident of this, but someone who knows about these groebner_assure, singular_assure, ... etc should have a close look.
What seems weird to me (and what caused the problems here) is that the ordering set in G.ord of an IdealGens object G may not agree with the ordering of the Singular ring in G.gens.Sx. So in particular G.gens.O may be a Gröbner basis with respect to G.ord, but the same polynomials in G.gens.S on the Singular side are "not a Gröbner basis" because G.gens.Sx has a different ordering.

[joschmitt]

I assume that we want singular_groebner_generators here and not just groebner_assure. The doc string of groebner_assure explains that one should not rely on the basis being defined on the Singular side.
I also assume that we need a global ordering here; otherwise one can change the true to false.

[joschmitt]

This should fix the main problem in #4018: singular_generators would not carry the information that the polynomials form a Gröbner basis with respect to monorder over to the Singular side.

[joschmitt]

This was wrong: the polynomial ring on the Singular side might have a monomial ordering different from I.ord.

[fingolfin]

We discussed this during triage. The plan is to wait until after the OSCAr workshop in two weeks, during which hopefully the underlying issue can be resolved.

If this for some reason doesn't happen, we can discuss again if we want to merge this for the time being until a "proper" fix can be implemented.

[ederc]

I think that the fixes @joschmitt provides in this PR are the correct ones. There is, as @wdecker pointed out, another problem with groebner_assure, but we can handle this in a different PR. So for me this PR is good to go.

[ederc]

For the above mentioned groebner_assure fix see #4077.

[joschmitt]

At last week's triage meeting, there was talk about an underlying issue and @fingolfin's summary above says that this pull request should wait, at least until the coding sprint next week. I don't want to restart the discussion; if somebody thinks this should be merged "as is", please do so. Otherwise I'm happy to talk about it next week.

[fingolfin]

Since the "deeper" issue is fixed and @ederc thinks this PR is good to go, I'll merge it now.