Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

markuspf · 2018-08-22T20:54:39Z

This PR renames QuaternionGroup to DicyclicGroup and QuaternionGroupCons to DicyclicGroupCons.

This is because these functions/constructors allow creation of groups that are not strictly speaking (generalised) quaternion groups: Their only requirement is that the given size of the group is a multiple of four, but the common definition of a generalised quaternion group is that its size is a power of 2 greater than 8.

The polycyclic package installs a method for QuaternionGroupCons which should probably be changed to DicyclicGroupCons as well, as it happily produces Dicyclic groups. I'll submit a PR for polycyclic, but have left the declaration of QuaternionGroupCons in here for the time being to not break code.

Additionally we enable existing documentation for IsDihedralGroup and IsQuaternionGroup, methods to recognise whether a given group is a dihedral group or a (generalised) quaternion group: Issue #2725 arose because one could create a group using QuaternionGroup for which IsQuaternionGroup was false.

Closes #2725

codecov · 2018-08-22T22:25:47Z

Codecov Report

Merging #2729 into master will decrease coverage by 0.3%.
The diff coverage is 93.05%.

@@            Coverage Diff             @@
##           master    #2729      +/-   ##
==========================================
- Coverage   75.81%    75.5%   -0.31%     
==========================================
  Files         481      481              
  Lines      241520   244905    +3385     
==========================================
+ Hits       183104   184925    +1821     
- Misses      58416    59980    +1564

Impacted Files	Coverage Δ
grp/basicfp.gi	`99.07% <ø> (-0.93%)`	⬇️
grp/basicpcg.gi	`100% <ø> (ø)`	⬆️
grp/basicprm.gi	`100% <ø> (ø)`	⬆️
grp/basicmat.gi	`99.3% <100%> (ø)`	⬆️
grp/basic.gd	`97.9% <91.66%> (-2.1%)`	⬇️
lib/grpnames.gi	`87.24% <94.11%> (+0.04%)`	⬆️
src/listoper.c	`63.93% <0%> (-16.73%)`	⬇️
src/compiler.c	`71.44% <0%> (-15.77%)`	⬇️
src/vars.c	`74.48% <0%> (-12.39%)`	⬇️
src/exprs.c	`83.02% <0%> (-11.33%)`	⬇️
... and 15 more

…lisedQuaternionGroup

markuspf · 2018-08-29T09:35:26Z

This is still far from perfect; we don't have IsDycyclicGroup yet, and the recognition is in lib/grpnames.g{d,i} and construction is in grp/; I don't know whether we want to move anything anywhere anytime soon though (and I don't want this PR to baloon into a huge project).

fingolfin · 2018-08-29T19:31:30Z

grp/basic.gd

-##  <A>n</A> in the category given by the filter <A>filt</A>.  Here, <A>n</A>
-##  is a multiple of 4.
+##  constructs the dicyclic group of size <A>n</A> in the category given by the
+##  filter <A>filt</A>. Here, <A>n</A> is a multiple of 4.


This text seems to refer to implicitly refer to DicyclicGroup only, but is of course listed directly after QuaternionGroup. I'd rephrase to e.g. "DicyclicGroup constucts ... " and then perhaps insert one sentence that mentions QuaternionGroup does the same but is restricted to inputs which are powers of 2, and that is provided for backwards compatibility.

fingolfin · 2018-08-29T19:39:42Z

lib/grpnames.gd

@@ -455,6 +457,7 @@ InstallTrueMethod( IsGroup, IsDihedralGroup );
 #P  IsQuaternionGroup( <G> )
 #A  QuaternionGenerators( <G> )
 ##
+##  <#GAPDoc Label="IsQuaternionGroup">
 ##  <ManSection>
 ##  <Prop Name="IsQuaternionGroup" Arg="G"/>
 ##  <Attr Name="QuaternionGenerators" Arg="G"/>


This change and the one a bit above changes four functions from undocumented to documented -- that's important for release notes, but not mentioned in the PR title or description (and hence easy to miss when creating release notes). Please mention this change in the PR title or description.

But in addition, I am a bit concerned about this: documenting QuaternionGenerators will cause people want to use it. But AFAICT we don't provide any methods for computing this attribute -- it is only set as a side effect of IsQuaternionGroup. Same for DihedralGenerators.

That said, I am not actively opposed to this change, but I am wary about the seemingly carefree approach to making it (I am sure you actually did spend time thinking carefully about it, it just isn't that clear from what is visibly documented in the system).

fingolfin · 2018-08-30T14:05:55Z

lib/grpnames.gd

+DeclareAttribute( "GeneralisedQuaternionGenerators", IsGroup );
+# Backwards compatibility
+DeclareSynonym( "IsQuaternionGroup", IsGeneralisedQuaternionGroup );
+DeclareSynonym( "QuaternionGenerators", GeneralisedQuaternionGenerators );


Both probably should use DeclareSynonymAttr, so that HasFOO works.

fingolfin · 2018-08-30T14:07:33Z

lib/grpnames.gi

    fi; # now Zn is normal in G and Zn = < s | s^n = 1 >

    # choose t in G\Zn and check dihedral structure
    repeat t := Random(G); until not t in Zn;
    if not (Order(t) = 2 and ForAll(GeneratorsOfGroup(Zn),s->t*s*t*s=s^0))
-    then return false; fi;
+    then return fail; fi;

    # choose generator s of Zn
    repeat s := Random(Zn); until Order(s) = n;
    SetDihedralGenerators(G,[t,s]);


Remove SetDihedralGenerators call here?

…hedralGenerators

…tors

fingolfin

Some more optional remarks

fingolfin · 2018-08-30T19:06:57Z

grp/basic.gd

-##  <A>n</A> in the category given by the filter <A>filt</A>.  Here, <A>n</A>
-##  is a multiple of 4.
+##  <Ref Func="DicyclicGroup"/> constructs the dicyclic group of size <A>n</A>
+##  in the category given by the #  filter <A>filt</A>. Here, <A>n</A> is a


Oops, something went wrong here rewrapping? At least there is a # in the middle of the line.

fingolfin · 2018-08-30T19:07:44Z

grp/basic.gd

-##  is a multiple of 4.
+##  <Ref Func="DicyclicGroup"/> constructs the dicyclic group of size <A>n</A>
+##  in the category given by the #  filter <A>filt</A>. Here, <A>n</A> is a
+##  multiple of 4. <Ref Func="QuaternionGroup"/> is a synonym for


Optional: Perhaps instead of "... n is a multiple of 4" this should say "... n should/must/... be a multiple of 4"?

fingolfin · 2018-08-30T19:10:27Z

grp/basic.gd

+    elif Length(args) = 3 then
+        res[1] := args[1];
+        res[2] := args[2];
+        res[3] := args[3];


Optional: how about res := args; instead?

fingolfin · 2018-08-30T19:10:31Z

grp/basic.gd

+        size := res[2];
+    elif Length(args) = 2 then
+        res[1] := args[1];
+        res[2] := args[2];


Optional: how about res := args; instead?

fingolfin · 2018-08-30T19:12:56Z

grp/basic.gd

+
+    if Length(args) = 1 then
+        res[1] := IsPcGroup;
+        res[2] := args[1];


All of this comment is optional.

How about res := [ IsPcGroup, args[1] ]; instead?

And the three places setting size could all be removed, if instead after the fi; in line 415, you add size := res[Length(res)].

With all those change,s you can also remove the initial res := [], and the function is much more compact.

I'd be happy to implement that in a commit I push on here; but we can also just the code leave it as it is now, after all, this is now just tweaking the color of the bikeshed ;-).

fingolfin · 2018-08-30T20:00:54Z

grp/basic.gd

+            ErrorNoReturn("usage: <size> must be a power of 2 and at least 8");
+        elif quaternion = "warn" then
+            Info(InfoWarning, 1, "Warning: QuaternionGroup called with <size> ", size,
+                                 " which is less than 8, or not a power of 2.");


Optional: we could be less mysterious and just tell the user explicitly what the problem is (also, I'd remove the trailing period, which is unusual in info messages):

elif quaternion = "warn" then if not IsPrimePowerInt(size) then Info(InfoWarning, 1, "Warning: QuaternionGroup called with <size> ", size, " which is not a power of 2"); else Info(InfoWarning, 1, "Warning: QuaternionGroup called with <size> ", size, " which is less than 8"); fi;

fingolfin · 2018-08-30T20:02:27Z

lib/grpnames.gd

+##    If it is, methods may set the attribute <Ref Attr="GeneralisedQuaternionGenerators" />
+##    to [<A>t</A>,<A>s</A>], where <A>t</A> and <A>s</A> are two elements such that <A>G</A> =
+##    <M>\langle t, s | s^{(2^k)} = 1, t^2 = s^{(2^k-1)}, s^t = s^{-1} \rangle</M>.
+##    <Ref Prop="IsQuaternionGroup"/> and <Ref Attr="QuaternionGenerators" /> are


Optional: perhaps start this sentence with the "The synonyms ..." ?

fingolfin · 2018-08-30T20:04:02Z

lib/grpnames.gi

+
+    gens := DoComputeDihedralGenerators(G);
+    if gens = fail then
+        ErrorNoReturn("G is not a dihedral group");


Optional: insert before the error: SetIsDihedralGroup(G, false);. Or insert before the if the following: SetIsDihedralGroup(G, gens <> fail); (and then remove the other call to SetIsDihedralGroup below). The same can of course be done in the quaternion case.

fingolfin · 2018-08-30T20:06:14Z

lib/grpnames.gi

+function(G)
+    local gens;
+
+    if not HasDihedralGenerators(G) then


Options: InstallTrueMethod(IsDihedralGroup, HasDihedralGenerators); in the .gd file, and then remove this if (the method can only be called if the dihedral generators are not (yet) known; though even if my claim was wrong, clearly it wouldn't be wrong to recompute them, only inefficient)

Of course the same could be done in the quaternion case.

fingolfin · 2018-08-30T22:45:30Z

grp/basic.gd

+    local size;
+
+    if Length(args) = 1 then
+        res := [ IsPcGroup, args[1] ];


This now errors out, because the local res is missing. Either add it back, or simply replace res by args (args is not used anymore later).

The other test failure seems to be due to the changed warning. Perhaps this tidbit is helpful (it's been for me, at least):

TestDirectory("tst/testinstall", rec(rewriteToFile:=true));

markuspf force-pushed the quaternion-fix-2 branch from 6337f4a to 00008ba Compare August 22, 2018 22:24

markuspf force-pushed the quaternion-fix-2 branch 3 times, most recently from fd3bbbd to 739a1a4 Compare August 24, 2018 23:21

markuspf added 3 commits August 25, 2018 00:33

Rename QuaternionGroup to DicyclicGroup

5f00480

Introduce parameter checks for DicyclicGroup, QuaternionGroup, Genera…

5a18ca6

…lisedQuaternionGroup

Update and add tests

bdea973

markuspf force-pushed the quaternion-fix-2 branch from 739a1a4 to bdea973 Compare August 24, 2018 23:35

Hook up some documentation for IsDihedralGroup and IsQuaternionGroup

243f947

markuspf removed do not comment yet PRs on which the author does not yet want any comment (e.g. only submitted for test results) do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Aug 29, 2018

markuspf requested review from fingolfin and hulpke August 29, 2018 09:36

hulpke approved these changes Aug 29, 2018

View reviewed changes

fingolfin reviewed Aug 29, 2018

View reviewed changes

markuspf changed the title ~~Rename QuaternionGroup to DicyclicGroup~~ Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup Aug 30, 2018

fingolfin reviewed Aug 30, 2018

View reviewed changes

Markus Pfeiffer added 2 commits August 30, 2018 15:45

Update documentation, install methods for QuaternionGenerators and Di…

69b4026

…hedralGenerators

Add some tests for DihedralGenerators and GeneralisedQuaternionGenera…

3092c37

…tors

markuspf force-pushed the quaternion-fix-2 branch from 473d04b to 3092c37 Compare August 30, 2018 14:46

fingolfin approved these changes Aug 30, 2018

View reviewed changes

Beautify GRPLIB_DicyclicParameterCheck

a3a0471

fingolfin reviewed Aug 30, 2018

View reviewed changes

Tweak Dihedral/GeneralisedQuaternion recognition

f4605ad

markuspf force-pushed the quaternion-fix-2 branch from b96934c to f4605ad Compare September 4, 2018 08:14

markuspf merged commit 16202bc into gap-system:master Sep 14, 2018

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019

olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

markuspf commented Aug 22, 2018 •

edited

Loading

codecov bot commented Aug 22, 2018 •

edited

Loading

markuspf commented Aug 29, 2018

fingolfin Aug 29, 2018

fingolfin Aug 29, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin left a comment

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

fingolfin Aug 30, 2018

Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

Conversation

markuspf commented Aug 22, 2018 • edited Loading

codecov bot commented Aug 22, 2018 • edited Loading

Codecov Report

markuspf commented Aug 29, 2018

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

fingolfin left a 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

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

markuspf commented Aug 22, 2018 •

edited

Loading

codecov bot commented Aug 22, 2018 •

edited

Loading