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

Add IsAbelian filters to IsSolvableGroup and DerivedSeries #706

Merged
merged 1 commit into from
Dec 7, 2016
Merged

Add IsAbelian filters to IsSolvableGroup and DerivedSeries #706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 39 additions & 7 deletions lib/grp.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,10 @@ InstallImmediateMethod( IsPerfectGroup,
0,
grp -> not IsAbelian( grp ) );

InstallMethod( IsPerfectGroup, "for groups having abelian invariants",
[ IsGroup and HasAbelianInvariants ],
grp -> Length( AbelianInvariants( grp ) ) = 0 );

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a clear win to me.

Copy link
Member

Choose a reason for hiding this comment

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

This function is not currently covered by the tests.

Indeed, I just looked into it and noticed that there is an almost identical method being installed for groups in the filter IsSubgroupFpGroup in grpf.gi, line 910.

The difference if of course that this method gets invoked even if they are not yet computed.

Which leaves me wondering how to create a group for which this method gets triggered...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fingolfin Ok, so we need a group that is not created as a subgroup of an fp-group, does not know if it is finite but does know its abelian invariants. Perhaps some PSL group constructed by generators over some infinite field? I am not sure how to do that in GAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fingolfin I did some poking around in the library. As far as I can tell, currently AbelianInvariants can only be computed for (subgroups of) fp-groups or for all groups, but then an IsFinite check is immediately forced. (Or it can be computed for Pcp-groups, but that uses the polycyclic package.) So currently I do not see a way to define a group for which we can compute AbelianInvariants and this line would run before any other for IsPerfectGroup.

InstallMethod( IsPerfectGroup,
"method for finite groups",
[ IsGroup and IsFinite ],
Expand Down Expand Up @@ -533,13 +537,22 @@ InstallMethod( IsSolvableGroup,
"generic method for groups",
[ IsGroup ],
function ( G )
local S; # derived series of <G>
local S, # derived series of <G>
isAbelian, # true if <G> is abelian
isSolvable; # true if <G> is solvable

# compute the derived series of <G>
S := DerivedSeriesOfGroup( G );

# the group is solvable if the derived series reaches the trivial group
return IsTrivial( S[ Length( S ) ] );
isSolvable := IsTrivial( S[ Length( S ) ] );

# set IsAbelian filter
isAbelian := isSolvable and Length( S ) <= 2;
Assert(3, IsAbelian(G) = isAbelian);
SetIsAbelian(G, isAbelian);

return isSolvable;
end );
Copy link
Member

Choose a reason for hiding this comment

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

This seems correct and fine, but I found it a bit hard to read and verify. How about something like this instead?

isSolvable := IsTrivial( S[ Length( S ) ] );
isAbelian := isSolvable and Length( S ) <= 2;
Assert(2, IsAbelian(G) = isAbelian);
SetIsAbelian(G, isAbelian);
return isSolvable;

I find that a lot easier to understand and verify, and it reduces code duplication.



Expand Down Expand Up @@ -900,16 +913,35 @@ InstallMethod( DerivedSeriesOfGroup,
D := DerivedSubgroup( G );

while
# we don't know that the group has no generators
(not HasGeneratorsOfGroup(S[Length(S)]) or
Length(GeneratorsOfGroup(S[Length(S)]))>0) and
( (not HasAbelianInvariants(S[Length(S)]) and D <> S[ Length(S) ]) or
Length(AbelianInvariants(S[Length(S)]))>0) do
(not HasIsTrivial(S[Length(S)]) or
not IsTrivial(S[Length(S)])) and
(
(not HasIsPerfectGroup(S[Length(S)]) and
not HasAbelianInvariants(S[Length(S)]) and D <> S[ Length(S) ]) or
(HasIsPerfectGroup(S[Length(S)]) and not IsPerfectGroup(S[Length(S)]))
or (HasAbelianInvariants(S[Length(S)])
and Length(AbelianInvariants(S[Length(S)])) > 0)
) do
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, this makes my head hurt... :). But it seems valid, and the code it replaces is not nicer, so no fundamental objection. Simplifying that doesn't seem easy... Hmmm

Perhaps we should add some functions like these:

IsAttributeKnownAndEqualTo := function(obj, attribute, value)
  return Tester(attribute)(obj) and attribute(obj) = value;
end;
IsKnownToBeTrivial := obj -> IsAttributeKnownAndEqualTo(obj, IsTrivial, true);

Then the code here could be changed to this (I made another change to avoid writing S[Length(S)] all the time; I didn't check this code, so I might have messed that up. But I hope my intent is clear):

S :=[];
D := G;
while not IsKnownToBeTrivial(D) and
         not IsAttributeKnownAndEqualTo(D, IsPerfectGroup, true) and
         not IsAttributeKnownAndEqualTo(D, AbelianInvariants, [] do
  Add( S, D );
  Info( InfoGroup, 2, "DerivedSeriesOfGroup: step ", Length(S) );
  D := DerivedSubgroup( D );
od;
Add( S, D );

I think there are other places in the GAP library which would become more readable with IsAttributeKnownAndEqualTo

Copy link
Member

Choose a reason for hiding this comment

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

There is one thing I wonder about, though: What if the group D (resp. S[Length(S)] does not "know" it is trivial, but actually is trivial... don't we then end up with a list S which contains two copies of the trivial group at the end? Same for the group being perfect...
(This is pertinent to all variants of the code: the current one, hungaborhorvath's version, and my proposed variant...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fingolfin
Should not IsAttributeKnownAndEqualTo, and similarly IsKnownToBeTrivial, go to some other file, like oper1.gi? Anyway, this seems like more than what this PR is for. I do not mind to wait with this PR until such functions are available, but such a useful function would be lost in this PR which is about something else.

BTW, your code really is not entirely the same as the original, but I get the idea. In any case, the code runs fine for e.g. Group((1,2,3),(3,4,5)), and the key is the D <> S[ Length(S) ]) part of the code.

Add( S, D );
Info( InfoGroup, 2, "DerivedSeriesOfGroup: step ", Length(S) );
D := DerivedSubgroup( D );
od;

# set filters if the last term is known to be trivial
if HasIsTrivial(S[Length(S)]) and IsTrivial(S[Length(S)]) then
SetIsSolvableGroup(G, true);
if Length(S) <=2 then
Assert(2, IsAbelian(G));
SetIsAbelian(G, true);
fi;
fi;

# set IsAbelian filter if length of derived series is more than 2
if Length(S) > 2 then
Assert(2, not IsAbelian(G));
SetIsAbelian(G, false);
fi;

Copy link
Member

Choose a reason for hiding this comment

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

How about simplifying this chunk similar to what I proposed for IsSolvableGroup ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not entirely the same, because here we do not want to force an IsAbelian or IsSolvable check. The only simplification I see now (it is late, though....) is to use IsAttributeKnownAndEqualTo.

# return the series when it becomes stable
return S;
end );
Expand Down
56 changes: 56 additions & 0 deletions tst/testinstall/opers/IsSolvableGroup.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
gap> START_TEST("IsSolvableGroup.tst");
gap> List(AllGroups(120), IsSolvableGroup);
[ true, true, true, true, false, true, true, true, true, true, true, true,
true, true, true, true, true, true, true, true, true, true, true, true,
true, true, true, true, true, true, true, true, true, false, false, true,
true, true, true, true, true, true, true, true, true, true, true ]
gap> List(AllTransitiveGroups(DegreeAction, 8), IsSolvable);
[ true, true, true, true, true, true, true, true, true, true, true, true,
true, true, true, true, true, true, true, true, true, true, true, true,
true, true, true, true, true, true, true, true, true, true, true, true,
false, true, true, true, true, true, false, true, true, true, true, false,
false, false ]
gap> IsSolvable(DihedralGroup(24));
true
gap> IsSolvable(DihedralGroup(IsFpGroup,24));
true
gap> DerivedSeries(Group(()));
[ Group(()) ]
gap> G := Group((6,7,8,9,10),(8,9,10),(1,2)(6,7),(1,2,3,4)(6,7,8,9));;
gap> Length(DerivedSeriesOfGroup(G));
4
gap> HasIsSolvableGroup(G) and not IsSolvable(G) and HasIsAbelian(G) and not IsAbelian(G);
true
gap> IsSolvableGroup(AbelianGroup([2,3,4,5,6,7,8,9,10]));
true
gap> HasIsSolvableGroup(AbelianGroup(IsFpGroup,[2,3,4,5,6,7,8,9,10]));
true
gap> IsSolvableGroup(AbelianGroup(IsFpGroup,[2,3,4,5,6,7,8,9,10]));
true
gap> IsSolvableGroup(Group(()));
true
gap> A := AbelianGroup([3,3,3]);; H := AutomorphismGroup(A);;
gap> B := SylowSubgroup(H, 13);; G := SemidirectProduct(B, A);;
gap> HasIsSolvableGroup(G) and IsSolvable(G);
true

## some fp-groups
## The following four tests check whether the current IsSolvable method using
## DerivedSeriesOfGroup indeed adds IsAbelian whenever it is appropriate. If
## later new methods for IsSolvable are added, these tests may fail. Then
## these four tests need to be modified accordingly.
gap> F := FreeGroup("r", "s");; r := F.1;; s := F.2;;
gap> G := F/[s^2, s*r*s*r];;
gap> IsSolvable(G) and HasIsAbelian(G) and not IsAbelian(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 line of the test, and the three similar ones following below, seem not quite right to me: they implicitly assume that IsSolvable is computed in one specific way (namely by computing the derived series). But at least hypothetically, I could come up with a much better way to compute solvability of fp-groups, say by only inspecting the relators. If that "superior" method was added to GAP, this test might suddenly fail, in case my method does not detect abelianess.

So, wouldn't it be better to explicitly call DerivedSeriesOfGroup(G) beforehand, as what we really are testing here is that computing the derived series automatically detects both solvability and abelianess...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that if a new method comes, then these tests may fail. However, after I checked the examples, I am now pretty confident that I wanted to check whether the method in this PR adds the appropriate IsSolvable and IsAbelian flags. The DerivedSeriesOfGroup should not (and does not) add most of these flags, mainly because it stops due to empty AbelianInvariants, instead of knowing that the last entry is trivial.

I would rather prefer to write a comment into the test file about these examples explaining this situation. But of course if you still prefer otherwise, I can run DerivedSeriesOfGroup first, and then check for the necessary flags, but that would not test whether the IsSolvable method in this PR really adds the appropriate flags.

Copy link
Member

Choose a reason for hiding this comment

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

A comment would be fine. Just so that if it ever fails due to another change (and it may never do so), then whoever has to fix the test at least understands why it is the way it is.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments to these tests.

true
gap> F := FreeGroup("a", "b", "c", "d");; a := F.1;; b := F.2;; c := F.3;; d:= F.4;;
gap> G := F/[ a^2, b^2, a*b*a^(-1)*b^(-1), c, d ];;
gap> IsSolvable(G) and HasIsAbelian(G) and IsAbelian(G);
true
gap> G := F/[ a^2, b^2, c^2, (a*b)^3, (b*c)^3, (a*c)^2, d ];;
gap> IsSolvable(G) and HasIsAbelian(G) and not IsAbelian(G);
true
gap> G := F/[ a^2, b^2, c^2, d^2, (a*b)^3, (b*c)^3, (c*d)^3, (a*c)^2, (a*d)^2, (b*d)^2 ];;
gap> not IsSolvable(G) and HasIsAbelian(G) and not IsAbelian(G);
true
gap> STOP_TEST("IsSolvableGroup.tst", 10000);
Copy link
Member

Choose a reason for hiding this comment

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

Tests, yay!!!