Nemocas
Inconsistent canonical_unit
The following seems inconsistent (it was reported in the comments of #575).
julia> using AbstractAlgebra
julia> S, x = PolynomialRing(ZZ, "x")
(Univariate Polynomial Ring in x over Integers, x)
julia> F = FractionField(S)
Fraction field of Univariate Polynomial Ring in x over Integers
julia> canonical_unit(F())
0
julia> canonical_unit(S())
1
It used to be the case that canonical_unit(0) == 0. In fact the documentation indicates that this is still the case for all fields.
However the docs are inconsistent as they say that the function can just return 1 when it is impractical to implement the function. Perhaps it should return 0 at 0. Or perhaps there is a justification for making it return 1 at 0. Personally I think that just hides bugs (like the one I just fixed on #575).
Either way, we should be more consistent about this. See also point 1 of #552
I'd be in favour of canonical_unit always returning a unit... and call teh default for fields (x ->x) an error
On Thu, Jun 10, 2021 at 05:11:43AM -0700, wbhart wrote:
The following seems inconsistent (it was reported in the comments of #575).
It used to be the case that canonical_unit(0) == 0. In fact the documentation indicates that this is still the case for all fields.
However the docs are inconsistent as they say that the function can just return 1 when it is impractical to implement the function. Perhaps it should return 0 at 0. Or perhaps there is a justification for making it return 1 at 0. Personally I think that just hides bugs (like the one I just fixed on #575).
Either way, we should be more consistent about this. See also point 1 of #552
-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/894
Yeah but that is problematic for inexact rings where checking for zero may not be possible, e.g. Calcium and Arb objects need to have it defined.
Is there any really good reason to change the definition other than that it says "unit" in the name of the function?
On Thu, Jun 10, 2021 at 05:49:53AM -0700, wbhart wrote:
Yeah but that is problematic for inexact rings where checking for zero may not be possible, e.g. Calcium and Arb objects need to have it defined.
Is there any really good reason to change the definition other than that it says "unit" in the name of the function?
Almost every useage will either invert the result or divide by it to normalise the input. For inexact fields, 1 might be correct...
Think HNF for example, canonical_unit make it unique (in Z) (granted: here I will never call it on zero)
I guess the 0 will have to be dealt with: either in all uses of canonical_unit or in canonical_unit.
Pest vs Cholera?
-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/894#issuecomment-858593454
I don't think the function should ever be called on zero. That's why the cost of checking if the value is zero (even if it is not) is too high for inexact rings.
It's designed for canonicalisation, which means dividing by the return value. Therefore it doesn't make sense that it ever be called on zero. If it is, the right thing will happen (a divide by zero error, indicating the code is faulty).
What I'm saying is that the cases where we test for zero should be minimised, which means calling code should do it if necessary. In most use cases it isn't necessary because you are dividing by the canonical unit of a denominator of a fraction (its original intended use), which is by definition not zero, so no need to check.
Related Question (I think). The Documentation indicates that canonical_unit of a Generic.FieldElem should just return the element itself (perhaps this also means for zero too).
Nevertheless, it looks like a fallback method like
canonical_unit(x::Generic.FieldElem) = x
Is not implemented. Is there an issue with this?
julia> using AbstractAlgebra
julia> K, t = LaurentSeriesField(GF(5), 5, "t")
(Laurent series field in t over Finite field F_5, t + O(t^6))
julia> canonical_unit(K(1))
ERROR: MethodError: no method matching canonical_unit(::AbstractAlgebra.Generic.LaurentSeriesFieldElem{AbstractAlgebra.GFElem{Int64}})
Closest candidates are:
canonical_unit(::PolynomialElem) at ~/.julia/dev/AbstractAlgebra/src/Poly.jl:469
canonical_unit(::MatrixElem) at ~/.julia/dev/AbstractAlgebra/src/Matrix.jl:358
canonical_unit(::FracElem) at ~/.julia/dev/AbstractAlgebra/src/Fraction.jl:105
...
Stacktrace:
[1] top-level scope
@ REPL[2]:1
julia> methods(canonical_unit)
# 15 methods for generic function "canonical_unit":
[1] canonical_unit(x::PolynomialElem) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/Poly.jl:469
[2] canonical_unit(a::MatrixElem) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/Matrix.jl:358
[3] canonical_unit(a::FracElem) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/Fraction.jl:105
[4] canonical_unit(p::AbstractAlgebra.Generic.UnivPoly) in AbstractAlgebra.Generic at /Users/f004584/.julia/dev/AbstractAlgebra/src/generic/UnivPoly.jl:280
[5] canonical_unit(a::LocElem{T}) where T<:RingElem in AbstractAlgebra.Generic at /Users/f004584/.julia/dev/AbstractAlgebra/src/generic/Misc/Localization.jl:412
[6] canonical_unit(x::ResElem{<:Union{RingElem, Integer}}) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/Residue.jl:104
[7] canonical_unit(a::AbstractFloat) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/julia/Float.jl:41
[8] canonical_unit(p::AbstractAlgebra.Generic.LaurentPolyWrap) in AbstractAlgebra.Generic at /Users/f004584/.julia/dev/AbstractAlgebra/src/generic/LaurentPoly.jl:214
[9] canonical_unit(x::AbstractAlgebra.LaurentPolyElem) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/algorithms/LaurentPoly.jl:200
[10] canonical_unit(x::AbstractAlgebra.GFElem) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/julia/GF.jl:122
[11] canonical_unit(a::Rational) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/julia/Rational.jl:51
[12] canonical_unit(a::T) where T<:Integer in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/julia/Integer.jl:44
[13] canonical_unit(x::AbstractAlgebra.ResFieldElem{<:Union{RingElem, Integer}}) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/ResidueField.jl:113
[14] canonical_unit(x::MPolyElem) in AbstractAlgebra at /Users/f004584/.julia/dev/AbstractAlgebra/src/MPoly.jl:508
[15] canonical_unit(a::AbstractAlgebra.Generic.Rat) in AbstractAlgebra.Generic at /Users/f004584/.julia/dev/AbstractAlgebra/src/generic/RationalFunctionField.jl:125
