Fix radical for ideals in polynomial rings over the integers

[HechtiDerLachs]

Resolves #5175.

[thofma]

It seems that doctests need to be adjusted.

[fingolfin]

I think this comment should be attached to the next method, as the method it documents right now does not call into Singular at all?

[fingolfin]

Looks plausible to me but someone from geometry should review & approve :-)

[wdecker]

I cannot (yet) see, why the proposed change causes the changes in the doctest example which makes the tests fails. The difference is, that now some redundant generators are removed. @HechtiDerLachs?

[thofma]

I think they just got reordered?

[wdecker]

I think they just got reordered?

Yes, you are right. But still, what caused this reordering?

[HechtiDerLachs]

what caused this reordering?

Before, we were using a preprocessing step which attempted to eliminate variables where applicable. This apparently does not work over the integers, so it has been cut off, leading to different results as the radical is now tentatively computed in a different ring.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.85%. Comparing base (7f7a868) to head (a121cff).
⚠️ Report is 77 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5185   +/-   ##
=======================================
  Coverage   84.84%   84.85%           
=======================================
  Files         710      710           
  Lines       95517    95519    +2     
=======================================
+ Hits        81045    81049    +4     
+ Misses      14472    14470    -2     

Files with missing lines	Coverage Δ
src/Rings/mpoly-ideals.jl	91.87% <ø> (ø)
src/Rings/mpolyquo-localizations.jl	75.64% <100.00%> (+0.03%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wdecker]

what caused this reordering?

Before, we were using a preprocessing step which attempted to eliminate variables where applicable. This apparently does not work over the integers, so it has been cut off, leading to different results as the radical is now tentatively computed in a different ring.

Not quite. The difference arises from how we define the identity map as shown in the example below:

julia> R, (x,y) = polynomial_ring(ZZ, [:x, :y]);

julia> I = ideal(R, [x^2,y^2]);

julia> L, _ = quo(R, I);

julia> f = identity_map(L);

julia> f(x^2)
x^2

julia> g = hom(L, L, [x, y]);

julia> g(x^2)
0

julia> g == f
true

@thofma Any comment?

[HechtiDerLachs]

Not quite. The difference arises from how we define the identity map as shown in the example below:

I do not understand the concern. Both versions of the identity map are correct. The difference observed in your example stems from different selections of the representative used for printing. I suspect f to not be of type MPolyAnyMap and use a different dispatch for mapping elements. Yet, I do not see anything that's wrong with this.

The given generators for the radical ideal in one of the doctests appear in different order compared to the earlier version. What's wrong with that? What is the issue? What is the concern?

As far as I can see, we do not even get a mathematically different result in the doctest's examples. While the example provided in the issue now works correctly.

[thofma]

I agree with @HechtiDerLachs. Everything is mathematically correct now, so we should get this in sooner than later.

The thing with identity_map(L) doing funny things is one of the complaints in #5188. It is actually not constructing the object you would expect (try asking kernel of it).

[wdecker]

I did not want to hold up things, but wanted to be sure that there is no hidden problem. I understand now.

[thofma]

Thanks @HechtiDerLachs