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

Jump to bottom

Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

Merged
merged 8 commits into from
Sep 14, 2018
Merged

Rename QuaternionGroup to DicyclicGroup, Document IsDihedralGroup and IsQuaternionGroup #2729

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5f00480
Rename QuaternionGroup to DicyclicGroup
markuspf Aug 22, 2018
5a18ca6
Introduce parameter checks for DicyclicGroup, QuaternionGroup, Genera…
markuspf Aug 22, 2018
bdea973
Update and add tests
markuspf Aug 22, 2018
243f947
Hook up some documentation for IsDihedralGroup and IsQuaternionGroup
Aug 29, 2018
69b4026
Update documentation, install methods for QuaternionGenerators and Di…
Aug 30, 2018
3092c37
Add some tests for DihedralGenerators and GeneralisedQuaternionGenera…
Aug 30, 2018
a3a0471
Beautify GRPLIB_DicyclicParameterCheck
markuspf Aug 30, 2018
f4605ad
Tweak Dihedral/GeneralisedQuaternion recognition
markuspf Aug 30, 2018
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
4 changes: 3 additions & 1 deletion doc/ref/grplib.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ calculations.
<#Include Label="ElementaryAbelianGroup">
<#Include Label="FreeAbelianGroup">
<#Include Label="DihedralGroup">
<#Include Label="QuaternionGroup">
<#Include Label="IsDihedralGroup">
<#Include Label="DicyclicGroup">
<#Include Label="IsQuaternionGroup">
<#Include Label="ExtraspecialGroup">
<#Include Label="AlternatingGroup">
<#Include Label="SymmetricGroup">
Expand Down
108 changes: 83 additions & 25 deletions grp/basic.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,64 +343,122 @@ end );

#############################################################################
##
#O QuaternionGroupCons( <filter>, <n> )
#O DicyclicGroupCons( <filter>, <n> )
##
## <ManSection>
## <Oper Name="QuaternionGroupCons" Arg='filter, n'/>
## <Oper Name="DicyclicGroupCons" Arg='filter, n'/>
##
## <Description>
## </Description>
## </ManSection>
##
DeclareConstructor( "QuaternionGroupCons", [ IsGroup, IsInt ] );
DeclareConstructor( "DicyclicGroupCons", [ IsGroup, IsInt ] );
DeclareSynonym("QuaternionGroupCons", DicyclicGroupCons);


#############################################################################
##
#F QuaternionGroup( [<filt>, ]<n> ) . . . . . . . quaternion group of order <n>
#F DicyclicGroup( [<filt>, [<field>, ] ]<n> ) Dicyclic group of order <n>
##
## <#GAPDoc Label="QuaternionGroup">
## <#GAPDoc Label="DicyclicGroup">
## <ManSection>
## <Func Name="QuaternionGroup" Arg='[filt, ]n'/>
## <Func Name="DicyclicGroup" Arg='[filt, ]n'/>
## <Func Name="DicyclicGroup" Arg='[filt, [field, ]]n'/>
## <Func Name="QuaternionGroup" Arg='[filt, [field, ]]n'/>
##
## <Description>
## constructs the generalized quaternion group (or 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.
## 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

## If <A>filt</A> is not given it defaults to <Ref Func="IsPcGroup"/>.
## For more information on possible values of <A>filt</A> see section
## (<Ref Sect="Basic Groups"/>).
## Methods are also available for permutation and matrix groups (of minimal
## degree and minimal dimension in coprime characteristic).
## <P/>
## <Example><![CDATA[
## gap> QuaternionGroup(32);
## <pc group of size 32 with 5 generators>
## gap> DicyclicGroup(24);
## <pc group of size 24 with 4 generators>
## gap> g:=QuaternionGroup(IsMatrixGroup,CF(16),32);
## Group([ [ [ 0, 1 ], [ -1, 0 ] ], [ [ E(16), 0 ], [ 0, -E(16)^7 ] ] ])
## ]]></Example>
## </Description>
## </ManSection>
## <#/GAPDoc>
##
BindGlobal( "QuaternionGroup", function ( arg )
BindGlobal( "GRPLIB_DicyclicParameterCheck",
function(args, quaternion)
local res, size;

res := [];

if Length(args) = 1 then
res[1] := IsPcGroup;
res[2] := args[1];
Copy link
Member

Choose a reason for hiding this comment

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

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 ;-).

size := res[2];
elif Length(args) = 2 then
res[1] := args[1];
res[2] := args[2];
Copy link
Member

Choose a reason for hiding this comment

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

Optional: how about res := args; instead?

size := res[2];
elif Length(args) = 3 then
res[1] := args[1];
res[2] := args[2];
res[3] := args[3];
Copy link
Member

Choose a reason for hiding this comment

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

Optional: how about res := args; instead?

size := res[3];

if not IsField(res[2]) then
ErrorNoReturn("usage: <field> must be a field");
fi;
else
ErrorNoReturn("usage: DicyclicGroup( [<filter>, [<field>, ] ] <size> )");
fi;

if Length(arg) = 1 then
return QuaternionGroupCons( IsPcGroup, arg[1] );
elif IsOperation(arg[1]) then
if Length(arg) = 2 then
return QuaternionGroupCons( arg[1], arg[2] );
elif Length(arg) = 3 then
# some filters require extra arguments, e.g. IsMatrixGroup + field
return QuaternionGroupCons( arg[1], arg[2], arg[3] );
if not IsFilter(res[1]) then
ErrorNoReturn("usage: <filter> must be a filter");
fi;
fi;
Error( "usage: QuaternionGroup( [<filter>, ]<size> )" );

end );
if not (IsPosInt(size) and (size mod 4 = 0)) then
ErrorNoReturn("usage: <size> must be a positive integer divisible by 4");
fi;

if not IsPrimePowerInt(size) or size < 8 then
if quaternion = "error" then
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.");
Copy link
Member

Choose a reason for hiding this comment

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

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;

fi;
fi;

return res;
end);

BindGlobal( "DicyclicGroup",
function(args...)
local res;

res := GRPLIB_DicyclicParameterCheck(args, "");

return CallFuncList(DicyclicGroupCons, res);
end);

BindGlobal( "QuaternionGroup",
function(args...)
local res;

DeclareSynonym( "DicyclicGroup", QuaternionGroup );
res := GRPLIB_DicyclicParameterCheck(args, "warn");

return CallFuncList(DicyclicGroupCons, res);
end);

BindGlobal( "GeneralisedQuaternionGroup",
function(args...)
local res, grp;

res := GRPLIB_DicyclicParameterCheck(args, "error");
grp := CallFuncList(DicyclicGroupCons, res);
SetIsQuaternionGroup(grp, true);

return grp;
end);


#############################################################################
Expand Down
4 changes: 2 additions & 2 deletions grp/basicfp.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,9 @@ end );

#############################################################################
##
#M QuaternionGroupCons( <IsFpGroup and IsFinite>, <n> )
#M DicyclicGroupCons( <IsFpGroup and IsFinite>, <n> )
##
InstallMethod( QuaternionGroupCons,
InstallMethod( DicyclicGroupCons,
"fp group",
true,
[ IsFpGroup and IsFinite,
Expand Down
12 changes: 6 additions & 6 deletions grp/basicmat.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,23 +68,23 @@ end );

#############################################################################
##
#M QuaternionGroupCons( <IsMatrixGroup>, <n> )
#M DicyclicGroupCons( <IsMatrixGroup>, <n> )
##
InstallMethod( QuaternionGroupCons,
InstallMethod( DicyclicGroupCons,
"matrix group for default field",
true,
[ IsMatrixGroup and IsFinite,
IsInt and IsPosRat ],
0,
function( filter, n )
return QuaternionGroup( filter, Rationals, n );
return DicyclicGroup( filter, Rationals, n );
end );

#############################################################################
##
#M QuaternionGroupCons( <IsMatrixGroup>, <field>, <n> )
#M DicyclicGroupCons( <IsMatrixGroup>, <field>, <n> )
##
InstallOtherMethod( QuaternionGroupCons,
InstallOtherMethod( DicyclicGroupCons,
"matrix group for given field",
true,
[ IsMatrixGroup and IsFinite,
Expand All @@ -103,7 +103,7 @@ function( filter, fld, n )
one := 1;
elif Characteristic( fld ) = 0 or (0 = n mod Characteristic( fld ))
then # XXX: regular rep is not minimal
grp := QuaternionGroup( IsPermGroup, n );
grp := DicyclicGroup( IsPermGroup, n );
grp := Group( List( GeneratorsOfGroup( grp ), prm -> PermutationMat( prm, NrMovedPoints( grp ), fld ) ) );
SetSize( grp, n );
return grp;
Expand Down
4 changes: 2 additions & 2 deletions grp/basicpcg.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,9 @@ end );

#############################################################################
##
#M QuaternionGroupCons( <IsPcGroup and IsFinite>, <n> )
#M DicyclicGroupCons( <IsPcGroup and IsFinite>, <n> )
##
InstallMethod( QuaternionGroupCons,
InstallMethod( DicyclicGroupCons,
"pc group",
true,
[ IsPcGroup and IsFinite,
Expand Down
4 changes: 2 additions & 2 deletions grp/basicprm.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,9 @@ InstallMethod( DihedralGroupCons,

#############################################################################
##
#M QuaternionGroupCons( <IsPermGroup>, <4n> )
#M DicyclicGroupCons( <IsPermGroup>, <4n> )
##
InstallMethod( QuaternionGroupCons,
InstallMethod( DicyclicGroupCons,
"perm. group",
true,
[ IsPermGroup, IsPosInt ], 0,
Expand Down
6 changes: 3 additions & 3 deletions lib/grp.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3220,7 +3220,7 @@ DeclareOperation("CentralizerModulo", [IsGroup,IsGroup,IsObject]);
## <M>U_1:= <A>G</A></M>,
## <M>U_i:= [<A>G</A>, U_{{i-1}}] U_{{i-1}}^{<A>p</A>}</M>.
## <Example><![CDATA[
## gap> g:=QuaternionGroup(12);;
## gap> g:=DicyclicGroup(12);;
## gap> PCentralSeries(g,2);
## [ <pc group of size 12 with 3 generators>, Group([ y3, y*y3 ]), Group([ y*y3 ]) ]
## gap> g:=SymmetricGroup(4);;
Expand Down Expand Up @@ -3250,7 +3250,7 @@ KeyDependentOperation( "PCentralSeries", IsGroup, IsPosInt, "prime" );
## <Ref Func="PCentralSeries"/>.
## <P/>
## <Example><![CDATA[
## gap> g:=QuaternionGroup(12);;
## gap> g:=DicyclicGroup(12);;
## gap> PRump(g,2) = PCentralSeries(g,2)[2];
## true
## gap> g:=SymmetricGroup(4);;
Expand Down Expand Up @@ -3279,7 +3279,7 @@ KeyDependentOperation( "PRump", IsGroup, IsPosInt, "prime" );
## It is the core of a Sylow <A>p</A>-subgroup of <A>G</A>,
## see <Ref Func="Core"/>.
## <Example><![CDATA[
## gap> g:=QuaternionGroup(12);;
## gap> g:=DicyclicGroup(12);;
## gap> PCore(g,2);
## Group([ y3 ])
## gap> PCore(g,2) = Core(g,SylowSubgroup(g,2));
Expand Down
8 changes: 6 additions & 2 deletions lib/grpnames.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ DeclareSynonym( "DecompositionTypes", DecompositionTypesOfGroup );
#P IsDihedralGroup( <G> )
#A DihedralGenerators( <G> )
##
## <#GAPDoc Label="IsDihedralGroup">
## <ManSection>
## <Prop Name="IsDihedralGroup" Arg="G"/>
## <Attr Name="DihedralGenerators" Arg="G"/>
Expand All @@ -441,9 +442,10 @@ DeclareSynonym( "DecompositionTypes", DecompositionTypesOfGroup );
## Indicates whether the group <A>G</A> is a dihedral group.
## If it is, methods may set the attribute <C>DihedralGenerators</C> to
## [<A>t</A>,<A>s</A>], where <A>t</A> and <A>s</A> are two elements such
## that <A>G</A> = <M><A>t, s | t^2 = s^n = 1, s^t = s^-1</A></M>.
## that <A>G</A> = <M>\langle t, s | t^2 = s^n = 1, s^t = s^{-1} \rangle</M>.
## </Description>
## </ManSection>
## <#/GAPDoc>
##
DeclareProperty( "IsDihedralGroup", IsGroup );
DeclareAttribute( "DihedralGenerators", IsGroup );
Expand All @@ -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"/>
Copy link
Member

Choose a reason for hiding this comment

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

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).

Expand All @@ -464,9 +467,10 @@ InstallTrueMethod( IsGroup, IsDihedralGroup );
## of size <M>N = 2^(k+1)</M>, <M>k >= 2</M>. If it is, methods may set
## the attribute <C>QuaternionGenerators</C> to [<A>t</A>,<A>s</A>],
## where <A>t</A> and <A>s</A> are two elements such that <A>G</A> =
## <M><A>t, s | s^(2^k) = 1, t^2 = s^(2^k-1), s^t = s^-1</A></M>.
## <M>\langle t, s | s^{(2^k)} = 1, t^2 = s^{(2^k-1)}, s^t = s^{-1} \rangle</M>.
## </Description>
## </ManSection>
## <#/GAPDoc>
##
DeclareProperty( "IsQuaternionGroup", IsGroup );
DeclareAttribute( "QuaternionGenerators", IsGroup );
Expand Down
Loading