`canonical_unit` does not always return a unit

[lgoettgens]

In my understanding of the documentation of the function canonical_unit (cf. #1153), this should always return a unit to allow for some canonicalization of an element x up to units via inv(canonical_unit(x)) * x. This, however, does not work in all cases. Consider the following examples:

julia> x = ZZ(2);
julia> inv(canonical_unit(x))*x
2

julia> x = ZZ(-2);
julia> inv(canonical_unit(x))*x
2

julia> x = ZZ(0);
julia> inv(canonical_unit(x))*x
0

julia> x = QQ(2);
julia> inv(canonical_unit(x))*x
1

julia> x = QQ(-2);
julia> inv(canonical_unit(x))*x
1

# everything as expected until here
julia> x = QQ(0);
julia> inv(canonical_unit(x))*x
ERROR: Element not invertible
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] inv(a::fmpq)
   @ Nemo ~/.julia/packages/Nemo/SxIUb/src/flint/fmpq.jl:472
 [3] top-level scope
   @ REPL[64]:1

This is due to the fact that canonical_unit(QQ(0)) is 0 and thus in particular not a unit. In this sense, I find the naming canonical_unit wildly misleading, if it isn't even assured of returning a unit. My proposal is to change its behaviour to ALWAYS return a unit. In the case of e.g. a zero as input, this unit could be chosen arbitrarily (but at least it is a unit), but for the sake of simplicity I would suggest to return 1 in these cases.

[wbhart]

Note that this is essentially a duplicate of https://github.com/Nemocas/AbstractAlgebra.jl/issues/894

It is mentioned there that deciding if something is zero is not easy to do generically.

The original intention of canonical_unit was to provide a way of canonicalising fractions so that the denominator is as "simple" as possible for printing reasons, and so that fractions have some kind of canonical form.

In the case of fractions of polynomials, it is the leading coefficients that are canonicalised if I recall correctly.

As our fractions have up until now required a viable gcdx function this has limited the rings we can support, and in particular denominators with zero make no sense, so what the canonical_unit function returns in this case is irrelevant.

However, we are apparently soon introducing the total ring of fractions of a ring that has zero divisors and no doubt the current canonical_unit behaviour will not be sufficient.

However, note that there are rings where determining whether something is a unit is expensive and so on.

Having said that, I think canonical_unit(QQ(0)) should return 1. I think the current behaviour is a bug.

I doubt that multiplying by the inverse of the canonical_unit is going to be the right way to proceed. There are rings without an inv function that might be supported in future. I would simply use divexact in generic code, as this is more likely to exist.

Also see https://github.com/Nemocas/AbstractAlgebra.jl/issues/552 for an alternative proposal for what canonical_unit should do. However, no final decision has been made that I am aware of.

[lgoettgens]

I totally agree with using divexact, but somehow this didn't come to my mind when writing the issue.

After reading the related issues, everyone involved once wrote that the current behavior canonical_unit(QQ(0)) == QQ(0) is not right / is a bug. For rings, where it is not easy to decide if an element is zero, the documentation already has a solution

For some rings, it is completely impractical to implement this function, in which case it may return 1 in the given ring.

[wbhart]

It may be possible to implement the function, but impossible to determine if a value is zero (e.g. in the arb/acb modules for example). The comment in the documentation is for rings where it is completely impractical to implement the function.

Anyway, we are agreed that the current behaviour for QQ(0) is a bug.