Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add constructor for Dicyclic groups and alias for Quaternion groups #4661

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add constructor for Dicyclic groups and alias for Quaternion groups #4661

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
76b93a0
Add dicyclic group constructor
jamesnohilly Feb 27, 2025
18008ed
Change quaternion_group to be an alias of dicyclic_group
jamesnohilly Feb 27, 2025
fb5dda0
Add test for dicyclic_group
jamesnohilly Feb 28, 2025
38a463e
Add changes from review
jamesnohilly Feb 28, 2025
3d757e0
Fix format issue
jamesnohilly Mar 3, 2025
e07d330
Add converse tests and example
jamesnohilly Mar 3, 2025
05ac3ec
Merge branch 'master' into jn/dicyclic
lgoettgens Mar 12, 2025
36a690c
Merge docstring and alias constructors
jamesnohilly Mar 31, 2025
1eb2831
Implement IsDicyclicGroup from GAP
jamesnohilly Apr 1, 2025
83cfa4b
Seperate is_dicyclic and is_quaternion
jamesnohilly Apr 1, 2025
b09e726
Remove GAP attribute for is_dicyclic
jamesnohilly Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 120 additions & 73 deletions src/Groups/group_constructors.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,11 @@ end
# Delegating to the GAP constructor via `_gap_filter` does not work here.
function abelian_group(::Type{TG}, v::Vector{T}) where TG <: Union{PcGroup, SubPcGroup} where T <: IntegerUnion
if 0 in v
# if 0 in v || (TG == PcGroup && any(!is_prime, v))
#TODO: Currently GAP's IsPcpGroup groups run into problems
# already in the available Oscar tests,
# see https://github.com/gap-packages/polycyclic/issues/88,
# so we keep the code from the master branch here.
# if 0 in v || (TG == PcGroup && any(!is_prime, v))
#TODO: Currently GAP's IsPcpGroup groups run into problems
# already in the available Oscar tests,
# see https://github.com/gap-packages/polycyclic/issues/88,
# so we keep the code from the master branch here.
# We cannot construct an `IsPcGroup` group if some generator shall have
# order infinity or 1 or a composed number.
return TG(GAP.Globals.AbelianPcpGroup(length(v), GapObj(v; recursive = true)))
Expand Down Expand Up @@ -327,8 +327,8 @@ julia> order(g)
```
"""
function projective_general_linear_group(n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PGL(n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PGL(n, q))
end


Expand All @@ -350,8 +350,8 @@ julia> order(g)
```
"""
function projective_special_linear_group(n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PSL(n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PSL(n, q))
end


Expand All @@ -373,9 +373,9 @@ julia> order(g)
```
"""
function projective_symplectic_group(n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
@req iseven(n) "The dimension must be even"
return PermGroup(GAP.Globals.PSp(n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
@req iseven(n) "The dimension must be even"
return PermGroup(GAP.Globals.PSp(n, q))
end


Expand All @@ -397,8 +397,8 @@ julia> order(g)
```
"""
function projective_unitary_group(n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PGU(n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PGU(n, q))
end


Expand All @@ -420,8 +420,8 @@ julia> order(g)
```
"""
function projective_special_unitary_group(n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PSU(n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
return PermGroup(GAP.Globals.PSU(n, q))
end


Expand All @@ -444,15 +444,15 @@ julia> g = projective_orthogonal_group(3, 3); order(g)
```
"""
function projective_orthogonal_group(e::Int, n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
if e == 1 || e == -1
@req iseven(n) "The dimension must be even"
elseif e == 0
@req isodd(n) "The dimension must be odd"
else
throw(ArgumentError("Invalid description of projective orthogonal group"))
end
return PermGroup(GAP.Globals.PGO(e, n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
if e == 1 || e == -1
@req iseven(n) "The dimension must be even"
elseif e == 0
@req isodd(n) "The dimension must be odd"
else
throw(ArgumentError("Invalid description of projective orthogonal group"))
end
return PermGroup(GAP.Globals.PGO(e, n, q))
end

projective_orthogonal_group(n::Int, q::Int) = projective_orthogonal_group(0, n, q)
Expand All @@ -477,15 +477,15 @@ julia> g = projective_special_orthogonal_group(3, 3); order(g)
```
"""
function projective_special_orthogonal_group(e::Int, n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
if e == 1 || e == -1
@req iseven(n) "The dimension must be even"
elseif e == 0
@req isodd(n) "The dimension must be odd"
else
throw(ArgumentError("Invalid description of projective special orthogonal group"))
end
return PermGroup(GAP.Globals.PSO(e, n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
if e == 1 || e == -1
@req iseven(n) "The dimension must be even"
elseif e == 0
@req isodd(n) "The dimension must be odd"
else
throw(ArgumentError("Invalid description of projective special orthogonal group"))
end
return PermGroup(GAP.Globals.PSO(e, n, q))
end

projective_special_orthogonal_group(n::Int, q::Int) = projective_special_orthogonal_group(0, n, q)
Expand All @@ -510,15 +510,15 @@ julia> g = projective_omega_group(3, 3); order(g)
```
"""
function projective_omega_group(e::Int, n::Int, q::Int)
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
if e == 1 || e == -1
@req iseven(n) "The dimension must be even"
elseif e == 0
@req isodd(n) "The dimension must be odd"
else
throw(ArgumentError("Invalid description of projective orthogonal group"))
end
return PermGroup(GAP.Globals.POmega(e, n, q))
@req is_prime_power_with_data(q)[1] "The field size must be a prime power"
if e == 1 || e == -1
@req iseven(n) "The dimension must be even"
elseif e == 0
@req isodd(n) "The dimension must be odd"
else
throw(ArgumentError("Invalid description of projective orthogonal group"))
end
return PermGroup(GAP.Globals.POmega(e, n, q))
end

projective_omega_group(n::Int, q::Int) = projective_omega_group(0, n, q)
Expand Down Expand Up @@ -611,17 +611,17 @@ julia> gens(free_group([:a, :b], "x" => 1:2, 'y' => (1:2, 1:2)))
```
"""
function free_group(L::Vector{<:Symbol}; eltype::Symbol = :letter)
@req allunique(L) "generator names must be unique"
@req allunique(L) "generator names must be unique"
J = GapObj(L, recursive = true)
if eltype == :syllable
if eltype == :syllable
G = FPGroup(GAP.Globals.FreeGroup(J; FreeGroupFamilyType = GapObj("syllable"))::GapObj)
elseif eltype == :letter
G = FPGroup(GAP.Globals.FreeGroup(J)::GapObj)
else
error("eltype must be :letter or :syllable, not ", eltype)
end
GAP.Globals.SetRankOfFreeGroup(GapObj(G), length(J))
return G
elseif eltype == :letter
G = FPGroup(GAP.Globals.FreeGroup(J)::GapObj)
else
error("eltype must be :letter or :syllable, not ", eltype)
end
GAP.Globals.SetRankOfFreeGroup(GapObj(G), length(J))
return G
end

# HACK: we want to use `AbstractAlgebra.@varnames_interface` for free groups,
Expand All @@ -636,8 +636,8 @@ end
# `@free_group` by delegating to the `@_free_group` macros (plus some extra
# shenigans).
function _free_group(L::Vector{<:Symbol}; eltype::Symbol = :letter)
G = free_group(L; eltype)
return G, gens(G)
G = free_group(L; eltype)
return G, gens(G)
end

AbstractAlgebra.@varnames_interface _free_group(s)
Expand All @@ -649,7 +649,7 @@ free_group(; kw...) = _free_group(0; kw...)[1]
# HACK to get the default variable name stem `:f` instead of `:x`
# but also to insert validation for `n`.
function free_group(n::Int, s::VarName = :f; kw...)
@req n >= 0 "n must be a non-negative integer"
@req n >= 0 "n must be a non-negative integer"
_free_group(n, s; kw...)[1]
end

Expand Down Expand Up @@ -693,12 +693,12 @@ macro free_group(args...)
# if the arguments are varnames, put them into a vector before delegating
# to @_free_group
esc(quote
Oscar.@_free_group([$(args...)])
Oscar.@_free_group([$(args...)])
end)
else
# by default just delegate to `@_free_group`
esc(quote
Oscar.@_free_group($(args...))
Oscar.@_free_group($(args...))
end)
end
end
Expand All @@ -721,7 +721,7 @@ end
#end

function free_abelian_group(::Type{FPGroup}, n::Int)
return FPGroup(GAPWrap.FreeAbelianGroup(n)::GapObj)
return FPGroup(GAPWrap.FreeAbelianGroup(n)::GapObj)
end


Expand Down Expand Up @@ -824,9 +824,11 @@ false

Return the (generalized) quaternion group of order `n`,
as an instance of `T`,
where `n` is a power of 2 and `T` is in
where `n` is a multiple of 4 and `T` is in
{`PcGroup`, `SubPcGroup`, `PermGroup`,`FPGroup`, `SubFPGroup`}.

This is an alias of `dicyclic_group`.

# Examples
```jldoctest
julia> g = quaternion_group(8)
Expand All @@ -843,38 +845,83 @@ julia> relators(g)
r^2*s^-2
s^4
r^-1*s*r*s

```
"""
quaternion_group(n::IntegerUnion) = quaternion_group(PcGroup, n)

function quaternion_group(::Type{T}, n::IntegerUnion) where T <: GAPGroup
# FIXME: resolve naming: dicyclic vs (generalized) quaternion: only the
# former should be for any n divisible by 4; the latter only for powers of 2.
# see also debate on the GAP side (https://github.com/gap-system/gap/issues/2725)
quaternion_group(::Type{T}, n::IntegerUnion) where {T<:Union{GAPGroup,PcGroup,SubPcGroup}} =
dicyclic_group(T, n)

@doc raw"""
is_quaternion_group(G::GAPGroup)

Return `true` if `G` is isomorphic to a (generalized) quaternion group
of order $2^{k+1}, k \geq 2$, and `false` otherwise.

# Examples
```jldoctest
julia> is_quaternion_group(small_group(8, 3))
false

julia> is_quaternion_group(small_group(8, 4))
true
```
"""
@gapattribute is_quaternion_group(G::GAPGroup) =
GAP.Globals.IsGeneralisedQuaternionGroup(GapObj(G))::Bool

"""
dicyclic_group(::Type{T} = PcGroup, n::IntegerUnion)

Return the dicyclic group of order `n`, as an instance of `T`,
where `n` is a multiple of 4 and `T` is in
{`PcGroup`, `SubPcGroup`, `PermGroup`,`FPGroup`, `SubFPGroup`}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust this to fully mirror the quaternion_group docstring (including a @ref link in the other direction).

... or... perhaps we should instead really merge the two, like GAP does: show a single docstring that starts with

"""
    dicyclic_group(::Type{T} = PcGroup, n::IntegerUnion)
    quaternion_group(::Type{T} = PcGroup, n::IntegerUnion)

...

Then attach that to, say, quaternion_group, and turn dicyclic_group into a "true" alias by doing const dicyclic_group = quaternion_group (I think it'll then get the same docstring). Merge is_quaternion_group and is_dicyclic_group` similarly.

In the examples use only one (quaternion).

Then instead of This is an alias of dicyclic_group. write something more like this:

!!! note
    For historical reasons and backwards compatibility, `dicyclic_group` is an alias
    of `quaternion_group`. The two functions are fully identical. We recommend always
    using `quaternion_group`.

and then similar for the is_* function.

Thoughts @ThomasBreuer @thofma ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is just an alias, as a user I would not quite understand why you provide an alias that I am happy to find (because "dicyclic group" is the right name in my domain) and use, but then tell me not to use it.

But I don't have a strong opinion on this.

Copy link
Member

@ThomasBreuer ThomasBreuer Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of the discussion from issue #1630 is that it should be the other way round, that is, we recommend always using dicyclic_group.

And yes, I think it makes sense to have just one docstring for both names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with not making a recommendation, my main concern is that I think it'd be nice to mention why we have two names.


# Examples
```jldoctest
julia> g = dicyclic_group(12)
Pc group of order 12

julia> dicyclic_group(PermGroup, 12)
Permutation group of degree 12

julia> g = dicyclic_group(FPGroup, 12)
Finitely presented group of order 12

julia> relators(g)
3-element Vector{FPGroupElem}:
r^2*s^-3
s^6
r^-1*s*r*s
```
"""
dicyclic_group(n::IntegerUnion) = dicyclic_group(PcGroup, n)

function dicyclic_group(::Type{T}, n::IntegerUnion) where {T<:GAPGroup}
@assert iszero(mod(n, 4))
return T(GAP.Globals.QuaternionGroup(_gap_filter(T), n)::GapObj)
return T(GAP.Globals.DicyclicGroup(_gap_filter(T), n)::GapObj)
end

# Delegating to the GAP constructor via `_gap_filter` does not work here.
function quaternion_group(::Type{T}, n::IntegerUnion) where T <: Union{PcGroup, SubPcGroup}
function dicyclic_group(::Type{T}, n::IntegerUnion) where {T<:Union{PcGroup,SubPcGroup}}
@assert iszero(mod(n, 4))
return T(GAP.Globals.QuaternionGroup(GAP.Globals.IsPcGroup, n)::GapObj)
return T(GAP.Globals.DicyclicGroup(GAP.Globals.IsPcGroup, n)::GapObj)
end

@doc raw"""
is_quaternion_group(G::GAPGroup)
is_dicyclic_group(G::GAPGroup)

Return `true` if `G` is isomorphic to a (generalized) quaternion group
of order $2^{k+1}, k \geq 2$, and `false` otherwise.
Return `true` if `G` is isomorphic to a dicyclic group
of order $4n, n > 1$, and `false` otherwise.

# Examples
```jldoctest
julia> is_quaternion_group(small_group(8, 3))
julia> is_dicyclic_group(small_group(8, 3))
false

julia> is_quaternion_group(small_group(8, 4))
julia> is_dicyclic_group(small_group(8, 4))
true
```
"""
@gapattribute is_quaternion_group(G::GAPGroup) = GAP.Globals.IsQuaternionGroup(GapObj(G))::Bool
@gapattribute is_dicyclic_group(G::GAPGroup) =
GAP.Globals.IsQuaternionGroup(GapObj(G))::Bool
2 changes: 2 additions & 0 deletions src/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ export describe
export desimulate_valuation
export det
export diameter
export dicyclic_group
export dihedral_group
export dim
export dim_of_torusfactor
Expand Down Expand Up @@ -844,6 +845,7 @@ export is_coroot_with_index
export is_cyclic, has_is_cyclic, set_is_cyclic
export is_degenerate
export is_dense
export is_dicyclic_group, has_is_dicyclic_group, set_is_dicyclic_group
export is_dihedral_group, has_is_dihedral_group, set_is_dihedral_group
export is_dominant
export is_du_val_singularity
Expand Down
4 changes: 4 additions & 0 deletions test/Groups/constructors.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ end
@test isa(dihedral_group(PermGroup, 6), PermGroup)

@test is_quaternion_group(small_group(8, 4))
@test is_dicyclic_group(small_group(8, 4))
@test small_group_identification(small_group(8, 4)) == (8, 4)
@test isa(small_group(8, 4), PcGroup)
@test isa(small_group(60, 5), PermGroup)
Expand Down Expand Up @@ -170,6 +171,9 @@ end

Q8 = quaternion_group(8)
@test isa(Q8, PcGroup)

Dic12 = dicyclic_group(12)
@test isa(Dic12, PcGroup)

gl = GL(2, 3)
@test isa(gl, MatrixGroup)
Expand Down
Loading