Primary decomposition rerouting

[HechtiDerLachs]

A prototype for what @afkafkafk13 suggested and @wdecker requested in #3083 .

[thofma]

Why do you reroute nf_elem? Doesn't Singular automatically do something useful if you have a simple extension over Q or k?

[thofma]

I am not a big fan of the name _flatten_ring, because we already have flatten. Maybe we can find a more descriptive name.

[afkafkafk13]

Why do you reroute nf_elem? Doesn't Singular automatically do something useful if you have a simple extension over Q or k?

Yes and no. In general (in particular in the groebner bases context), it does.
But in the primary decomposition context, it is more desirable to go down all the way to QQ or k for internal reasons.

[codecov]

Codecov Report

Merging #3091 (39f8c4c) into master (503d48c) will increase coverage by 0.04%.
Report is 4 commits behind head on master.
The diff coverage is 91.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3091      +/-   ##
==========================================
+ Coverage   80.51%   80.55%   +0.04%     
==========================================
  Files         524      525       +1     
  Lines       70408    70635     +227     
==========================================
+ Hits        56689    56901     +212     
- Misses      13719    13734      +15     

Files	Coverage Δ
...erimental/Schemes/MorphismFromRationalFunctions.jl	40.87% <ø> (+0.09%)	⬆️
src/Rings/MPolyMap/MPolyAnyMap.jl	94.28% <100.00%> (+0.28%)	⬆️
src/Rings/mpoly-ideals.jl	94.43% <100.00%> (+1.41%)	⬆️
src/Rings/mpolyquo-localizations.jl	74.59% <81.81%> (+<0.01%)	⬆️
src/Rings/primary_decomposition_helpers.jl	87.31% <87.31%> (ø)

... and 3 files with indirect coverage changes

[HechtiDerLachs]

@ederc : There seems to be some hickup with algebraic solving in the tests here. I suppose it does not have to do with my changes. Do you agree?

[HechtiDerLachs]

I am not a big fan of the name _flatten_ring, because we already have flatten. Maybe we can find a more descriptive name.

Agreed. We will figure it out next week.

[thofma]

Since you will have to touch the code anyway, maybe you could also add some documentation in form of a comment (just the general idea, maybe at the top of src/Rings/primary_decomposition_helpers.jl).

[ederc]

I tested locally with Julia 1.6 and I cannot recreate the failing test. Also in PR #3090 the test does not fail which is strange.

[HechtiDerLachs]

If tests pass, this should be ready to go.

[thofma]

I still get incomprehensible errors for

julia> K, a = number_field([x - 1, x - 2]);

julia> Kt, t = K["t"];

julia> L, b = number_field(t - 1, "b");

julia> M, = number_field(t - 1, "b");

julia> Mx, = polynomial_ring(M, 2);

julia> primary_decomposition(ideal(Mx, [gen(Mx, 1)]))
ERROR: MethodError: no method matching _expand_coefficient_field(::MPolyQuoRing{AbstractAlgebra.Generic.MPoly{NfAbsNSElem}}; rec_depth::Int64)

This is a regression, since it is working fine on current master:

julia> Qx, x = QQ["x"]
(Univariate polynomial ring in x over QQ, x)

julia> K, a = number_field([x - 1, x - 2]);

julia> Kt, t = K["t"];

julia> L, b = number_field(t - 1, "b");

julia> M, = number_field(t - 1, "b");

julia> Mx, = polynomial_ring(M, 2);

julia> primary_decomposition(ideal(Mx, [gen(Mx, 1)]))
1-element Vector{Tuple{MPolyIdeal{AbstractAlgebra.Generic.MPoly{Hecke.NfRelElem{NfAbsNSElem}}}, MPolyIdeal{AbstractAlgebra.Generic.MPoly{Hecke.NfRelElem{NfAbsNSElem}}}}}:
 (ideal(x1), ideal(x1))

Catching this cannot be handled using the signature, so you need to use something like

_is_a_tower_we_can_handle(K::QQField) = true

_is_a_tower_we_can_handle(K::NumField) = (K isa AnticNumberField || K isa Oscar.Hecke.NfRel) && _is_a_tower_we_can_handle(base_field(K))

And then check inside the function and call the ordinary primary decomposition in case it returns false.

Alternatively, methods need to be added for conversion of the other field types.

[HechtiDerLachs]

What now? Can this be merged?

[thofma]

As I said on slack, I will review it today.

[thofma]

It's a shame we (I) did not implement hom(K::NumField, R::Ring) for arbitrary rings. It would have turned those x -> evaluate(x.data, theta) into neat hom(K, R_exp, theta).

[HechtiDerLachs]

Yes, that's what I would have needed. Let me know, should that change one day.

[thofma]

I will adjust it once thofma/Hecke.jl#1324 is merged.

[HechtiDerLachs]

Very good, thanks!