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
+137 −82
Open

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

Show file tree
Hide file tree
Changes from 2 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
73 changes: 59 additions & 14 deletions src/Groups/group_constructors.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,8 @@ as an instance of `T`,
where `n` is a power of 2 and `T` is in
Copy link
Collaborator

@thofma thofma Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
where `n` is a power of 2 and `T` is in
where `n` is a multiple of 4 and `T` is in

Copy link
Member

Choose a reason for hiding this comment

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

I think "power of 2" is what we want here, as that is how (generalized) quaternion groups are defined. For the function that does the exact same construction for "multiple of 4", there is dicyclic_group below. In particular, this distinction in the docstring matches to how is_quaternion_group works

Copy link
Collaborator

@thofma thofma Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I think that the consensus in #1630 is that both should be the same. Here are some reasons why I think this is the case:

  1. The definition with 2-powers is unnecessarily restrictive. There are plenty of references that construct quaternion groups of order 4n.
  2. There is now already code out there that is using the most general definition (that the order is any number $4n$), because this is what people were being told in Dicyclic groups #1630
  3. Since we don't do any quaternion_group(k) -> group of order 2^k or 2^(k+2) shenanigans, there is no reason to restrict to 2-powers.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I decided to just treat both commands as exact equals, any artificial restriction benefits nobody but can cause frustration

Copy link
Member

Choose a reason for hiding this comment

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

Note that e.g. Wikipedia allows "generalized quaternion groups" to have order $4k$.

{`PcGroup`, `SubPcGroup`, `PermGroup`,`FPGroup`, `SubFPGroup`}.

This is an alias of [`dicyclic_group`](@ref).

# Examples
```jldoctest
julia> g = quaternion_group(8)
Expand All @@ -843,38 +845,81 @@ 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 <: GAPGroup = dicyclic_group(T, n)
quaternion_group(::Type{T}, n::IntegerUnion) where T <: Union{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
Loading