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

jamesnohilly · 2025-02-27T13:38:13Z

This PR adds a group constructor for Dicyclic Groups (dicyclic_group) and changes the constructor for Quaternion Groups to be an alias of dicyclic_group.

This PR closes #1630

src/Groups/group_constructors.jl

thofma · 2025-02-27T13:59:47Z

src/Groups/group_constructors.jl

@@ -827,6 +827,8 @@ as an instance of `T`,
 where `n` is a power of 2 and `T` is in


Suggested change

where `n` is a power of 2 and `T` is in

where `n` is a multiple of 4 and `T` is in

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

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:

The definition with 2-powers is unnecessarily restrictive. There are plenty of references that construct quaternion groups of order 4n.

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

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.

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

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

src/Groups/group_constructors.jl

lgoettgens · 2025-02-27T15:43:08Z

src/Groups/group_constructors.jl

@@ -827,6 +827,8 @@ as an instance of `T`,
 where `n` is a power of 2 and `T` is in


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

src/Groups/group_constructors.jl

test/Groups/constructors.jl

src/Groups/group_constructors.jl

codecov · 2025-03-12T13:54:35Z

Codecov Report

Attention: Patch coverage is 90.19608% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.38%. Comparing base (4bccbc1) to head (05ac3ec).

Files with missing lines	Patch %	Lines
src/Groups/group_constructors.jl	90.19%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4661      +/-   ##
==========================================
- Coverage   84.38%   84.38%   -0.01%     
==========================================
  Files         673      673              
  Lines       90284    90287       +3     
==========================================
+ Hits        76185    76186       +1     
- Misses      14099    14101       +2

Files with missing lines	Coverage Δ
src/Groups/group_constructors.jl	`93.49% <90.19%> (+0.11%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thofma

Very nice, thanks. But some group theory person should have a look too.

src/Groups/group_constructors.jl

fingolfin · 2025-03-20T23:55:51Z

src/Groups/group_constructors.jl

+
+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`}.


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 ?

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.

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.

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.

src/Groups/group_constructors.jl

ThomasBreuer · 2025-03-21T10:16:41Z

src/Groups/group_constructors.jl

 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


I am getting confused.
GAP does the following:

gap> G:= DicyclicGroup(24);; gap> IsQuaternionGroup(G); false gap> G:= DicyclicGroup(32);; gap> IsQuaternionGroup(G); true

That is, both QuaternionGroup and DicyclicGroup can be used to create the groups in question,
but IsQuaternionGroup regards only the dicyclic groups of 2-power order as (generalized) quaternion groups.
In fact, GAP seems to have no function that checks whether a group is dicyclic.

As far as I see, the GAP function DoComputeGeneralisedQuaternionGenerators (which is called by IsQuaternionGroup) would work as an is_dicyclic check if one just omits the check for 2-power order.

ThomasBreuer · 2025-03-31T07:39:14Z

How shall we proceed?
Meanwhile gap-system/gap/pull/5966 got merged, do we want to wait until Oscar can use it, or do we want to add Oscar code that provides the same functionality?

jamesnohilly · 2025-03-31T08:03:10Z

I have now merged the docstrings of dicyclic_group and quaternion_group, with main preference on dicyclic_group as in #1630.

As for the is_{dicyclic,quaternion}_group issue, we could wait for a new GAP.jl release to use these new methods, however I am unsure. Adding this functionality to OSCAR and then eventually replacing is an option aswell.

fingolfin · 2025-04-02T13:28:51Z

src/Groups/group_constructors.jl

+  n = N//2
+  a = n//2


This creates rationals but since the numbers are even we are allowed to divide, so we should be able to do this:

Suggested change

n = N//2

a = n//2

n = quo(N,2)

a = quo(n,2)

I think you mean div instead of quo

fingolfin · 2025-04-02T13:30:34Z

src/Groups/group_constructors.jl

+  a = n//2
+
+  G1, _ = derived_subgroup(G)
+  !(is_cyclic(G1) && order(G1) == a) && return false


This may be a matter of taste, so take with a grain of salt; but I find this easier to understand:

Suggested change

!(is_cyclic(G1) && order(G1) == a) && return false

(is_cyclic(G1) && order(G1) == a) || return false

I read this as "either G1 is cyclic and has order a, or else we return false

Feelfree to ignore this!

fingolfin · 2025-04-02T13:43:40Z

src/Groups/group_constructors.jl

+  T = right_transversal(G, G1)
+  i = 1
+  while true
+    H = GAP.Globals.ClosureGroup(GapObj(G1), GapObj(T[i]))


This line motivated me to write issue #4777 ;-)

fingolfin · 2025-04-02T13:47:33Z

src/Groups/group_constructors.jl

+  i = 1
+  while true
+    H = GAP.Globals.ClosureGroup(GapObj(G1), GapObj(T[i]))
+    Zn, _ = Oscar._as_subgroup(G1, H)
+    i = i + 1
+    if (i > 4) || (is_cyclic(Zn) && order(Zn) == n)


I guess we could do away with i and just do this? But it would also make the code differ more from the GAP version, so perhaps not worth it anyway

Suggested change

i = 1

while true

H = GAP.Globals.ClosureGroup(GapObj(G1), GapObj(T[i]))

Zn, _ = Oscar._as_subgroup(G1, H)

i = i + 1

if (i > 4) || (is_cyclic(Zn) && order(Zn) == n)

@assert length(T) == 4

for x in T

H = GAP.Globals.ClosureGroup(GapObj(G1), GapObj(x))

Zn, _ = Oscar._as_subgroup(G1, H)

if is_cyclic(Zn) && order(Zn) == n

fingolfin · 2025-04-02T14:08:03Z

src/Groups/group_constructors.jl

+    of `dicyclic_group`. The two functions are fully identical. We recommend always
+    using `dicyclic_group`.


Suggested change

of `dicyclic_group`. The two functions are fully identical. We recommend always

using `dicyclic_group`.

of `dicyclic_group`. The two functions are fully identical.

jamesnohilly added 2 commits February 27, 2025 14:32

Add dicyclic group constructor

76b93a0

Change quaternion_group to be an alias of dicyclic_group

18008ed

jamesnohilly commented Feb 27, 2025

View reviewed changes

src/Groups/group_constructors.jl Outdated Show resolved Hide resolved

jamesnohilly commented Feb 27, 2025

View reviewed changes

src/Groups/group_constructors.jl Outdated Show resolved Hide resolved

thofma reviewed Feb 27, 2025

View reviewed changes

lgoettgens added the topic: groups label Feb 27, 2025

lgoettgens reviewed Feb 27, 2025

View reviewed changes

fingolfin added the enhancement New feature or request label Feb 27, 2025

jamesnohilly added 2 commits February 28, 2025 15:47

Add test for dicyclic_group

fb5dda0

Add changes from review

38a463e

thofma reviewed Mar 1, 2025

View reviewed changes

test/Groups/constructors.jl Show resolved Hide resolved

thofma reviewed Mar 1, 2025

View reviewed changes

src/Groups/group_constructors.jl Outdated Show resolved Hide resolved

jamesnohilly added 2 commits March 3, 2025 12:01

Fix format issue

3d757e0

Add converse tests and example

e07d330

jamesnohilly marked this pull request as ready for review March 3, 2025 17:29

Merge branch 'master' into jn/dicyclic

05ac3ec

lgoettgens requested review from fingolfin and thofma March 12, 2025 12:56

thofma approved these changes Mar 12, 2025

View reviewed changes

fingolfin reviewed Mar 20, 2025

View reviewed changes

ThomasBreuer reviewed Mar 21, 2025

View reviewed changes

src/Groups/group_constructors.jl Outdated Show resolved Hide resolved

ThomasBreuer reviewed Mar 21, 2025

View reviewed changes

ThomasBreuer mentioned this pull request Mar 28, 2025

add IsDicyclicGroup and DicyclicGenerators gap-system/gap#5966

Merged

Merge docstring and alias constructors

36a690c

jamesnohilly added 3 commits April 1, 2025 13:59

Implement IsDicyclicGroup from GAP

1eb2831

Seperate is_dicyclic and is_quaternion

83cfa4b

Remove GAP attribute for is_dicyclic

b09e726

fingolfin reviewed Apr 2, 2025

View reviewed changes

fingolfin approved these changes Apr 2, 2025

View reviewed changes

fingolfin reviewed Apr 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jamesnohilly commented Feb 27, 2025 •

edited

Loading

thofma Feb 27, 2025 •

edited

Loading

lgoettgens Feb 27, 2025

thofma Feb 27, 2025 •

edited

Loading

fingolfin Feb 27, 2025

fingolfin Mar 20, 2025

lgoettgens Feb 27, 2025

codecov bot commented Mar 12, 2025

thofma left a comment

fingolfin Mar 20, 2025

thofma Mar 21, 2025

ThomasBreuer Mar 21, 2025 •

edited

Loading

fingolfin Apr 2, 2025

ThomasBreuer Mar 21, 2025

ThomasBreuer Mar 21, 2025

ThomasBreuer commented Mar 31, 2025

jamesnohilly commented Mar 31, 2025

fingolfin Apr 2, 2025

lgoettgens Apr 3, 2025

fingolfin Apr 2, 2025

fingolfin Apr 2, 2025

fingolfin Apr 2, 2025

fingolfin Apr 2, 2025

		@@ -827,6 +827,8 @@ as an instance of `T`,
		where `n` is a power of 2 and `T` is in

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

	!(is_cyclic(G1) && order(G1) == a) && return false
	(is_cyclic(G1) && order(G1) == a) \|\| return false

		of `dicyclic_group`. The two functions are fully identical. We recommend always
		using `dicyclic_group`.

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

Are you sure you want to change the base?

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

Conversation

jamesnohilly commented Feb 27, 2025 • edited Loading

thofma Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 12, 2025

Codecov Report

thofma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBreuer Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBreuer commented Mar 31, 2025

jamesnohilly commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesnohilly commented Feb 27, 2025 •

edited

Loading

thofma Feb 27, 2025 •

edited

Loading

thofma Feb 27, 2025 •

edited

Loading

ThomasBreuer Mar 21, 2025 •

edited

Loading