Add conformance tests for MPolyRing, and fix bugs in ^ and is_unit for MPoly

[fingolfin]

No description provided.

[JohnAAbbott]

I avoided calling constant_coefficient because it might iterate over the terms in f, but then we iterate again over the terms in f in the main loop. Here is a partial demonstration:

julia> P,(x,y) = polynomial_ring(QQ, ["x","y"]);
julia> f = (2*x+3*y-4)^3 + (3*x-2*y-6)^3;
julia> f999 = f^999;
julia> @time constant_coefficient(f999);
  3.127883 seconds (17.98 M allocations: 5.133 GiB, 15.79% gc time)

I have just tried a more realistic test, which actually calls the code I wrote:

julia> P,(x,y) = polynomial_ring(ZZmod720, ["x","y"]);
julia> f = (2*x+3*y-4)^3 + (3*x-2*y-6)^3;
julia> f9999 = f^9999; # takes a long time
julia> @time constant_coefficient(f9999);
  0.052131 seconds (1.00 M allocations: 76.614 MiB, 18.73% gc time)
julia> @time is_unit(f9999)
  0.000009 seconds (1 allocation: 80 bytes)
false

So in this case, calling constant_coefficient will be a bit slower. That said, @fingolfin code is neater.

[fingolfin]

More importantly, my new code is less wrong ;-).

julia> R,_ = residue_ring(ZZ,ZZ(720));

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

julia> is_unit(30*x)
true

I am happy if somebody adjusts the code to be faster again, but to me correctness trumps speed.

[fingolfin]

OK I thought about it and I think I found a better way, we'll see.

[JohnAAbbott]

The version with constant_term_is_unit looks fine to me: i.e. should be both fast and correct

[fingolfin]

Tests fail inside pow_fps (ping @fieker):

  Expression: equality(a ^ 4, a * a * a * a)
  ArgumentError: not a unit
  Stacktrace:
    [1] inv(a::BigInt)
      @ AbstractAlgebra ~/Projekte/OSCAR/AbstractAlgebra.jl/src/julia/Integer.jl:242
    [2] divrem(f::AbstractAlgebra.Generic.Poly{BigInt}, g::AbstractAlgebra.Generic.Poly{BigInt})
      @ AbstractAlgebra ~/Projekte/OSCAR/AbstractAlgebra.jl/src/Poly.jl:1448
    [3] divrem
      @ ~/Projekte/OSCAR/AbstractAlgebra.jl/src/AbstractAlgebra.jl:65 [inlined]
    [4] gcdx(a::AbstractAlgebra.Generic.Poly{BigInt}, b::AbstractAlgebra.Generic.Poly{BigInt})
      @ AbstractAlgebra ~/Projekte/OSCAR/AbstractAlgebra.jl/src/algorithms/GenericFunctions.jl:306
    [5] gcdinv
      @ ~/Projekte/OSCAR/AbstractAlgebra.jl/src/algorithms/GenericFunctions.jl:321 [inlined]
    [6] invmod(a::AbstractAlgebra.Generic.Poly{BigInt}, m::AbstractAlgebra.Generic.Poly{BigInt})
      @ AbstractAlgebra ~/Projekte/OSCAR/AbstractAlgebra.jl/src/algorithms/GenericFunctions.jl:139
    [7] divides(a::EuclideanRingResidueRingElem{AbstractAlgebra.Generic.Poly{BigInt}}, b::EuclideanRingResidueRingElem{AbstractAlgebra.Generic.Poly{BigInt}})
      @ AbstractAlgebra ~/Projekte/OSCAR/AbstractAlgebra.jl/src/Residue.jl:373
    [8] #divexact#272
      @ ~/Projekte/OSCAR/AbstractAlgebra.jl/src/Residue.jl:352 [inlined]
    [9] divexact
      @ ~/Projekte/OSCAR/AbstractAlgebra.jl/src/Residue.jl:350 [inlined]
   [10] pow_fps(f::AbstractAlgebra.Generic.MPoly{EuclideanRingResidueRingElem{AbstractAlgebra.Generic.Poly{BigInt}}}, k::Int64, bits::Int64)
      @ AbstractAlgebra.Generic ~/Projekte/OSCAR/AbstractAlgebra.jl/src/generic/MPoly.jl:2265
   [11] ^(a::AbstractAlgebra.Generic.MPoly{EuclideanRingResidueRingElem{AbstractAlgebra.Generic.Poly{BigInt}}}, b::Int64)
      @ AbstractAlgebra.Generic ~/Projekte/OSCAR/AbstractAlgebra.jl/src/generic/MPoly.jl:2364
   [12] literal_pow
      @ ~/Projekte/OSCAR/AbstractAlgebra.jl/src/NCRings.jl:131 [inlined]

What is going on here is that there is test code starting out like this:

   S, x = polynomial_ring(ZZ, "x")
   T, = residue_ring(S, x^2 + 1)

Note that we take a quotient of Z[x] by an ideal -- but Z[x] is not an euclidean domain. But it gets handled by EuclideanRingResidueRing resp. EuclideanRingResidueRingElem -- and they try to compute gcdx on elements of Z[x] and unsurprisingly this can run into an error.

Which is part of my motivation for opening #1943 to discuss adding an is_euclidean_type trait.

But I digress: I can of course adjust the failing test to instead work over Q[x]. But I wonder about all the other tests in Residue-test.jl which test stuff on Z[x]... should they also be changed? Then again, the docs actually state:

AbstractAlgebra.jl provides modules, implemented in src/Residue.jl and
src/residue_field for residue rings and fields, respectively, over any
Euclidean domain (in practice most of the functionality is provided for GCD
domains that provide a meaningful GCD function) [...]

So it sounds as if this is almost kinda intentionally supported-but-with-some-pitfalls? urgh

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.23%. Comparing base (82c1b6a) to head (04625b5).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1950      +/-   ##
==========================================
+ Coverage   88.22%   88.23%   +0.01%     
==========================================
  Files         119      119              
  Lines       30425    30429       +4     
==========================================
+ Hits        26842    26849       +7     
+ Misses       3583     3580       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fingolfin]

Wonderful, this PR found a bug in Nemo:

function is_unit(a::fqPolyRepMPolyRingElem)
  return is_constant(a)
end