oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
338 stars 120 forks source link

`map_word` inconsistency for free group with zero generators #3796

Closed thofma closed 3 months ago

thofma commented 4 months ago
julia> FF = free_group(1)
Free group of rank 1

julia> map_word(one(FF), [-1])
1

julia> FF = free_group(0)
Free group of rank 0

julia> map_word(one(FF), Int[]) |> typeof
Nothing

I think it should error or also return 1.

@ThomasBreuer can you have a look?

ThomasBreuer commented 4 months ago

Yes, I will have a look.

fieker commented 4 months ago

On Mon, May 27, 2024 at 01:35:08AM -0700, Tommy Hofmann wrote:

julia> FF = free_group(1)
Free group of rank 1

julia> map_word(one(FF), [-1])
1

julia> FF = free_group(0)
Free group of rank 0

julia> map_word(one(FF), Int[]) |> typeof
Nothing

I think it should error or also return 1.

@ThomasBreuer can you have a look?

There is a fix

function map_word(v::Union{Vector{Int}, Vector{Pair{Int, Int}}, Vector{Any}}, genimgs::Vector; genimgs_inv::Vector = Vector(undef, length(genimgs)), init = nothing) length(genimgs) == 0 && @.*** length(v) == 0; return init)

-- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/3796 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

ThomasBreuer commented 3 months ago

@thofma If the array of generator images is empty then the type of this array does in general not determine a unique image, thus returning 1 would require a special treatment. The idea was to use the optional init argument in this situation. My interpretation of your suggestion is that we should throw an error if the array of generator images is empty and init is nothing. This makes sense.

@fieker If I understand your proposal right then you want to return init whenever v is empty and init is not nothing. Currently we return init only if the array of generator images is empty, i.e., if init is the only chance to create an image.

I regard @thofma's suggestion as an enhancement and @fieker's suggestion as a breaking change.

thofma commented 3 months ago

I would suggest to just make it error in the situation where we cannot do anything useful in general. The error message should tell me that I should make use of the ;init = ... keyword argument.

fieker commented 3 months ago

On Mon, May 27, 2024 at 06:30:42AM -0700, Thomas Breuer wrote:

@thofma If the array of generator images is empty then the type of this array does in general not determine a unique image, thus returning 1 would require a special treatment. The idea was to use the optional init argument in this situation. My interpretation of your suggestion is that we should throw an error if the array of generator images is empty and init is nothing. This makes sense.

@fieker If I understand your proposal right then you want to return init whenever v is empty and init is not nothing. Currently we return init only if the array of generator images is empty, i.e., if init is the only chance to create an image.

I regard @thofma's suggestion as an enhancement and @fieker's suggestion as a breaking change.

There is was a second change hidden:

  • length(v) == 0 && return one(genimgs[1]) This will crash if the codomain is not a group, hence one does not make sense (e.g. i Tommy's example we evaluate at Ints. This is where init has to be used - similar to prod and sum and reduce in general -- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/3796#issuecomment-2133492308 You are receiving this because you were mentioned.

Message ID: @.***>

ThomasBreuer commented 3 months ago

@fieker I do not catch the point, one(1) works, doesn't it? Of course map_word assumes that the given images can be multiplied and powered by integers. If one cannot be assumed, would genimgs[1]^0 be safer?

fieker commented 3 months ago

On Mon, May 27, 2024 at 07:26:49AM -0700, Thomas Breuer wrote:

@fieker I do not catch the point, one(1) works, doesn't it? I was using it where one() does not work, I think for s.th. like a number field where one(NumberField) has to fail Of course map_word assumes that the given images can be multiplied and powered by integers. If one cannot be assumed, would genimgs[1]^0 be safer? only of the lsit has length >0. init is porvided preciely for that - and should be used...

-- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/3796#issuecomment-2133595378 You are receiving this because you were mentioned.

Message ID: @.***>

ThomasBreuer commented 3 months ago

@fieker I am now thinking about a different situation: map_word can be used for images which may even support one but for which one may in fact be wrong. Take idempotent matrices such as m = [1 0; 0 0], then julia supports m^0 == [1 0; 0 1], but one might prefer [1 0; 0 0] as initial value. This use case can be modeled only if the init argument is used whenever it is different from nothing. (Similar situations occur when one evaluates polynomials.)

Such situations appear only if the images are not necessarily invertible. If map_word cannot deal with such "monoid" situations then we have to provide another function for that. Thus I think we should make this breaking change for map_word: Use init whenever it is given and the given word is empty.