AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

`canonical_unit` implementations differ from docs

Open lgoettgens opened this issue 2 years ago • 7 comments

I thought about adding some conformance checks for canonical_unit via adding

@test is_unit(canonical_unit(a))
@test equality(canonical_unit(a * b), canonical_unit(a) * canonical_unit(b))

after this line. The reasoning comes from the documentation.

However, this fails for basically all rings, and is even worse for some others (cf. #1166). The main reason being canonical_unit(0 * something) = canonical_unit(0) = 1 != 1 * canonical_unit(something) = canonical_unit(0) * canonical_unit(something).

To circumvent this problem, one could change the conformance test to

@test is_unit(canonical_unit(a))
@test iszero(a * b) || equality(canonical_unit(a * b), canonical_unit(a) * canonical_unit(b))

This still fails for some rings, e.g. Matrix rings, due to the implementation there (link to implementation).

I would suggest either making the implementations compliant with the documentation or modifying the documentation. Any thoughts on this?

lgoettgens avatar Jan 29 '23 19:01 lgoettgens

Thanks for summing up the mismatch between the documentation and the reality. There are some other aspects of canonical_unit that require changes that would including fixing this. So I am not sure it is a good idea to spend time on fixing this.

(For example, the function should return the inverse of what it returns now and/or the caonicalized value. It should also be optional, probably using a trait like CanCanonicalize (like it is done in the iterator interface https://docs.julialang.org/en/v1/manual/interfaces/). But this now turns into a bigger overhaul that no one has time right now.)

thofma avatar Jan 29 '23 21:01 thofma

Where is it stated that canonical_unit is a morphism? I don't think it is..

On Sun, 29 Jan 2023, 22:25 Tommy Hofmann, @.***> wrote:

Thanks for summing up the mismatch between the documentation and the reality. There are some other aspects of canonical_unit that require changes that would including fixing this. So I am not sure it is a good idea to spend time on fixing this.

(For example, the function should return the inverse of what it returns now and/or the caonicalized value. It should also be optional, probably using a trait like CanCanonicalize (like it is done in the iterator interface https://docs.julialang.org/en/v1/manual/interfaces/). But this now turns into a bigger overhaul that no one has time right now.)

— Reply to this email directly, view it on GitHub https://github.com/Nemocas/AbstractAlgebra.jl/issues/1254#issuecomment-1407773808, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36CV4BPVKRHD34V3MSILDWU3N45ANCNFSM6AAAAAAUKNHBV4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

fieker avatar Jan 30 '23 05:01 fieker

Found the counter example:z/8[X]: 6x+1 gets normalised by 3 to 2x+3 Multiply by 4, normalised by 1... you get 4 normalised by 1 again

On Mon, 30 Jan 2023, 06:54 Claus Fieker, @.***> wrote:

Where is it stated that canonical_unit is a morphism? I don't think it is..

On Sun, 29 Jan 2023, 22:25 Tommy Hofmann, @.***> wrote:

Thanks for summing up the mismatch between the documentation and the reality. There are some other aspects of canonical_unit that require changes that would including fixing this. So I am not sure it is a good idea to spend time on fixing this.

(For example, the function should return the inverse of what it returns now and/or the caonicalized value. It should also be optional, probably using a trait like CanCanonicalize (like it is done in the iterator interface https://docs.julialang.org/en/v1/manual/interfaces/). But this now turns into a bigger overhaul that no one has time right now.)

— Reply to this email directly, view it on GitHub https://github.com/Nemocas/AbstractAlgebra.jl/issues/1254#issuecomment-1407773808, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36CV4BPVKRHD34V3MSILDWU3N45ANCNFSM6AAAAAAUKNHBV4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

fieker avatar Jan 30 '23 07:01 fieker

OK, reading the doc, I think what is required/ suggested is that for a unit u, we have that canonical_unit(a)u == canonical_unit(au) whic his trivially canonical_unit(a) * canonical_unit(u) since for units canonical_unit(u) == u always.

In general this is tricky: for a Ring R we consider a system of representatives of R modulo units and canonical_unit more of less picks the rep. So canonical_unit(u) == u just means that one rep is 1. In geenral, I don't think one can impose any homomorphic properties on the representatives chosen....

fieker avatar Jan 30 '23 08:01 fieker

@lgoettgens where in the docs does it claim that canonical_unit is a homomorphism? I just couldn't find a place, but if there is one, it should indeed be fixed.

In the meantime, docs/src/ring_interface.md describes the identity @fieker mentions, where one factor is a unit.

fingolfin avatar Mar 10 '23 08:03 fingolfin

On Fri, Mar 10, 2023 at 12:05:49AM -0800, Max Horn wrote:

@lgoettgens where in the docs does it claim that canonical_unit is a homomorphism? I just couldn't find a place, but if there is one, it should indeed be fixed.

I don't think it can be a homomorphism at all: R = Z/12Z

then canonical_unit(R(2)) = 1 since gcd(12, 2) = 2 = 12 then canonical_unit(R(4)) = 1 since gcd(12, 4) = 4 = 14 then canonical_unit(R(8)) = 5 since gcd(12, 8) = 4 = 5*8

In the meantime, docs/src/ring_interface.md describes the identity @fieker mentions, where one factor is a unit.

-- Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/1254#issuecomment-1463422466 You are receiving this because you were mentioned.

Message ID: @.***>

fieker avatar Mar 10 '23 08:03 fieker

@lgoettgens where in the docs does it claim that canonical_unit is a homomorphism? I just couldn't find a place, but if there is one, it should indeed be fixed.

In the meantime, docs/src/ring_interface.md describes the identity @fieker mentions, where one factor is a unit.

It comes from the same place in stable/ring_interface/#Canonicalisation @fieker mentioned. I think I read it that way since it is not stated that u is supposed to be a unit.

lgoettgens avatar Mar 10 '23 10:03 lgoettgens