Fix leading_coefficient for zero polynomials

[SirToby25]

Currently, the Oscar implementation of leading_coefficient for zero MPolys is inconsistent with AbstractAlgebra. In AbstractAlgebra it is zero but in Oscar an error is raised. Additionally, this is neither the case for univariate zero polynomials nor any trailing coefficients.

julia> R, x = polynomial_ring(QQ, :x)
(Univariate polynomial ring in x over QQ, x)

julia> S, (a, b, c) = polynomial_ring(QQ, 3);

julia> leading_coefficient(zero(R))
0

julia> trailing_coefficient(zero(R))
0

julia> leading_coefficient(zero(S))
ERROR: ArgumentError: zero polynomial does not have a leading term
Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/AbstractAlgebra/M1vfh/src/Assertions.jl:602 [inlined]
 [2] index_of_leading_term(f::QQMPolyRingElem, ord::MonomialOrdering{QQMPolyRing})
   @ Oscar.Orderings ~/software/Oscar/src/Rings/orderings.jl:2075
 [3] #leading_coefficient#98
   @ ~/software/Oscar/src/Rings/mpoly.jl:1017 [inlined]
 [4] leading_coefficient(f::QQMPolyRingElem)
   @ Oscar ~/software/Oscar/src/Rings/mpoly.jl:1016
 [5] top-level scope
   @ REPL[14]:1

julia> trailing_coefficient(zero(S))
0

This pull request should fix this.

[wdecker]

This needs to be discussed.

[fingolfin]

The change looks fine to me and in line to how we define this method to work in AbstractAlgebra. But if there is need to discuss this at triage, so be it!

[wdecker]

There are several problems to be discussed here.

One problem is that this PR creates a lot of inconsistencies. For example, there are also functions like

leading_coefficient_and_exponent
leading_exponent
leading_exponent_vector
leading_monomial
leading_term

So given a polynomial which is zero, we now learn from calling leading_term and leading_coefficent that (as is mathematically correct) the zero polynomial does not have a leading term, while at the same time this nonexisting term has a coefficient which is zero. Or, by calling leading_coefficient_and_exponent and leading_coefficent, we learn that such a coefficient does not resp. does exist.

I do not know who introduced trailing_coefficient and where this is used, but indeed, the handling of the zero polynomial of this function does not fit into the original concept.

[wdecker]

Let me take this opportunity to point out that there are lots of serious inconsisties between AA and Oscar in the general context of what we discuss here. This is due to the fact that in AA, the concept of monomial orderings is way too rudimentary to be used in commutative algebra.

In AA, you can associate an internal_ordering to a multivariate polynomial ring according to which polynomials are stored. Possible orderings are :lex, :deglex, and :degrevlex, with :lex being the default.

In Oscar, you can additionally associate a default_ordering to a multivariate polynomial ring which is used in Gröbner basis related computations. Here, you can associate any possible ordering, with :degrevlex being the default.

As one consequence you have, for example, this:

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

julia> AbstractAlgebra.leading_coefficient(2*x+3*y^2)
2

julia> leading_coefficient(2*x+3*y^2)
3

Another consequence is that polynomials are printed according to the AA ordering, and not to the Oscar ordering which certainly can be confusing.

I do not claim that we should/can change all this, sometimes early unfortunate design decisions cannot be reversed. We have to live with these, so why creating new problems by "fixing" a minor inconsistency such as the leading coefficient of the zero polynomial. In any case I do not see a bug or unexpected error here: That this error should be thrown was decided following serious discussions and we could live with it for many years.

[jankoboehm]

It does not make sense to define the leading coefficient of the zero polynomial. Since the zero polynomial does not have any terms, it does not have a leading term, hence no leading coefficient (and obviously not a leading monomial or exponent vector).

The printing stuff we can't do much about (and probably should also not, since a polynomial does not have per se an ordering of terms).

[thofma]

Small historic remark: as we added the commutative algebra capabilities to Oscar, we discovered that some of the AbstractAlgebra conventions and implementations are not really suited. To mitigate this, in Oscar we have introduced in #1696 leading_coefficient etc., which do the "right" thing:

julia> Qx, (x, y) = QQ[:x, :y]
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> leading_coefficient(zero(Qx))
ERROR: ArgumentError: zero polynomial does not have a leading term
[...]

julia> AbstractAlgebra.leading_coefficient(zero(Qx))
0

• It seems that we forgot to do this for trailing_coefficient.
• The consistency with univariate polynomials is less important than the consistency within the multivariate polynomial world.

So this change here is not something we want to add back.

[fingolfin]

Another comment: should there be a fallback for trailing_coefficient in src/fallbacks.jl as well

@lgoettgens pointed out that universal polynomial rings also needed to be dealt with.... however this raises more questions and problems

in the end it might be easiest to also adjust AA to throw an error in leading/trailing coefficient for zero polynomials -- this would be a breaking change but we already plan a breaking release of AA to happen real soon now...

ping @thofma regarding the AA change bit...

[fingolfin]

@wdecker will fix trailing_coefficient in Oscar

[thofma]

I finally remembered to check with Magma. They do the opposite: throw an error in the univariate case and return zero in the multivariate case:

> Qx<x> := PolynomialRing(Rationals());
> LeadingCoefficient(0*x);

>> LeadingCoefficient(0*x);
                     ^
Runtime error in 'LeadingCoefficient': Argument 1 is not non-zero

> Qx<x, y> := PolynomialRing(Rationals(), 2);
> LeadingCoefficient(0*x);
0

So I would say we are in good company.

[fingolfin]

We changed the behavior in AA so that leading_coefficient of a zero polynomial throws an error, and this will be the behaviour universally in Oscar 1.5 (to be released today or tomorrow).

So I think this can be closed now.