Add IsAbelian filters to IsSolvableGroup and DerivedSeries #706

hungaborhorvath · 2016-04-02T11:48:53Z

IsAbelian filters are added to IsSolvableGroup and DerivedSeries.
IsSolvable filters are added to DerivedSeriesOfGroup.
Some immediate methods are installed for IsTrivial and IsPerfectGroup.
@hulpke's enhancement on DerivedSeriesOfGroup in commit e5eb173
about HasGeneratorsOfGroup and about HasAbelianInvariants are replaced
with HasIsTrivial and HasIsPerfectGroup (for which the corresponding
immediate methods are installed, see above).

hungaborhorvath · 2016-04-02T13:21:56Z

Ok, this is really embarassing. My current modifications apparently recovered a bug in somewhere in my own previous code for SylowSubgroups or HallSubgroups computation, and I cannot for the love of my life reproduce it by hand.

As far as I can see, the main change is in method selection for HallSubgroups (now GAP knows that the group is abelian and thus moves to the nilpotent method rather than the pcgs method), but I cannot track it properly, because typing of the commands in the nilpotent HallSubgroups method gives the correct result, not the incorrect one. Even more so, if I call HallSubgroup(G,[2,3]); without calling SylowSubgroup(G,2); first, then I receive the correct result.

Is there any way I can use profiling to have a file with all the commands run in the order they are actually called?

hungaborhorvath · 2016-04-02T15:29:07Z

Ok, so after #708 is merged, tests should not fail anymore.

hulpke · 2016-04-03T15:17:52Z

Why do we again have a multitude of immediate methods put in here?

Yes, they all look harmless, but I don't see them achieving much either and they contribute minuscule to the runtime of all other code.

hungaborhorvath · 2016-04-03T15:24:10Z

@hulpke I felt that you put in your checks for DerivedSeriesOfGroup to check whether we already know if the derived subgroup is trivial or perfect. This might be known for the group from other sources than what you put into the DerivedSeries algorithm. Thus, I put these checks into immediate methods (considering they only flip one bit and need to check very little, thus not too expensive), and replaced those checks with IsTrivial and IsPerfectGroup. I feel this better demonstrates my way of thinking about algorithms. I was hoping you are not against quick immediate methods for properties.

hulpke · 2016-04-03T15:41:20Z

@hungaborhorvath
I really don't like immediate methods at all. I understand that there are a few (very few, carefully chosen) situations where they genuinely help, but I object to adding them wholesale to try to catch arbitrary properties that are not set in the hope to have some code run particularly efficient for particular classes of groups (e.g. the trivial group) which rarely occur.

Immediate methods can be make debugging hard, as they jump in without a trace in the actual code being run.
Whenever a filter is set the immediate method mechanism gets triggered and needs to inspect the list of potentially applicable methods. For example a method for known size gets triggered every time a group happens to know its order. While it is just one line of code (plus the cost of calling), this will happen millions of times in functions such a subgroup lattice and thus will be noticeable.
A group with empty generating set actually does know already that it is trivial:

gap> g:=Group([],());;
gap> HasIsTrivial(g);
true

One of the methods thus is already superfluous.

hungaborhorvath · 2016-04-03T16:28:18Z

@hulpke Very well, I removed all three immediate methods and then rewrote the checks in DerivedSeriesOfGroup accordingly.

I am also wondering about the following example:

gap> F := FreeGroup("x", "y");; x := F.1;; y := F.2;;
gap> G := F/[x*y*x^(-1)*y^(-1), y^2];;

Calling IsSolvable here errors out after some time, but IsAbelian is immediate. Is there a way that this could also be recognized in DerivedSeries somehow?

fingolfin

All in all, I think this is a worthwhile contribution (and I am sorry it's been neglected since April sigh).

@hungaborhorvath I could totally understand if you are fed up by the delays and constant nitpicking :-/. But in case you are still with us (yay!!), perhaps you are interested in replying to my comments, and/or just rebasing your work on the latest master branch (that way, the code coverage test gets run, which will tell us if your tests fully exercise the new code).

Thankt you once more for your contributions and patience!

fingolfin · 2016-11-03T13:03:09Z

lib/grp.gi

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


This looks like a clear win to me.

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

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

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

fingolfin · 2016-11-03T13:05:50Z

lib/grp.gi

+ Assert(2, not IsAbelian(G));
+ SetIsAbelian(G, false);
+ return false;
+ fi;
 end );


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.

fingolfin · 2016-11-03T13:07:39Z

lib/grp.gi

+ Assert(2, not IsAbelian(G));
+ SetIsAbelian(G, false);
+ fi;
+


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

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.

fingolfin · 2016-11-03T13:23:53Z

lib/grp.gi

+ (HasIsPerfectGroup(S[Length(S)]) and not IsPerfectGroup(S[Length(S)]))
+ or (HasAbelianInvariants(S[Length(S)])
+ and Length(AbelianInvariants(S[Length(S)])) > 0)
+ ) do


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

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

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

fingolfin · 2016-11-03T13:26:24Z

tst/testinstall/opers/IsSolvableGroup.tst

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


Tests, yay!!!

fingolfin · 2016-11-04T23:23:42Z

One of the commits also has a typo in the commit message ("fo" instead of "for")

fingolfin · 2016-11-05T14:43:32Z

@hungaborhorvath ping, could you please rebase this PR, too?

hungaborhorvath · 2016-11-05T17:25:01Z

@fingolfin Yes, I will rebase it and make the changes you suggested. However, these are actual code changes, so I would like to take some time to make sure to get them right.

hungaborhorvath · 2016-11-05T23:19:10Z

@fingolfin I have rebased this, and also did one of the simplifications. As to the other, I am not sure that should be included in this PR or in a separate one.

codecov-io · 2016-11-05T23:45:53Z

Current coverage is 48.72% (diff: 96.00%)

Merging #706 into master will decrease coverage by 0.05%

@@             master       #706   diff @@
==========================================
  Files           424        424          
  Lines        222120     222141    +21   
  Methods        3429       3429          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         108343     108235   -108   
- Misses       113777     113906   +129   
  Partials          0          0

Powered by Codecov. Last update 5f18c3d...05c4635

fingolfin · 2016-11-09T12:36:07Z

lib/grp.gi

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


What is the point of this assert? It adds several seconds to your tests, as tests are run at assertion level 2 right now.

I'd either remove it, or increase the level to 3.

I did not know about the assertion level being increased for tests, thank you. Increased it to 3. There are several other places in grp.gi where I think very similar lines exist with assertion level 2.

fingolfin · 2016-11-09T12:40:48Z

tst/testinstall/opers/IsSolvableGroup.tst

+true
+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);


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

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.

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!

Added comments to these tests.

olexandr-konovalov · 2016-11-09T23:34:30Z

@hungaborhorvath regarding assertion levels, you may find this useful: #564

hungaborhorvath · 2016-11-10T06:02:10Z

Hm, now this branch has conflicts with the current master. I guess due to some merges. How can I resolve this?

fingolfin · 2016-11-10T11:41:02Z

@hungaborhorvath the conflict comes from an immediate method you added -- and then later on remove. If you squash your changes into fewer commits, or even just a single one, this will go. You can do this via interactive rebase. Run this command with your PR checked out (and no local modifications):

git rebase -i becaeff915dbaef518b68822ba51c34333d5ca98

This will open up an editor, and you can instruct it to s(quas) or f(ixup) some or all of your commits together. In this case, I think you could just squash everything into a single commit.

Once that worked, you should be able to rebase onto master again, and force-push the result.

hungaborhorvath · 2016-11-10T11:56:02Z

@fingolfin Hm, there is a problem. I did what you suggested (picked the first commit, squashed all the others, slightly rephrased the commit message). Then I rebased according to gap-system/master without any error messages, but somehow github complains about all checks failing.... Did I mess something up?

fingolfin · 2016-11-10T12:22:47Z

Seems I was mistaken and there is still a conflict (in particular, you didn't rebase onto the latest master branch version...).

If you rebase onto the latest master, you'll have to resolve the conflict. I can post some instructions how to do that later.

@hulpke

- IsAbelian filters are added to IsSolvableGroup and DerivedSeries. - IsSolvable filters are added to DerivedSeriesOfGroup. - @hulpke's enhancement on DerivedSeriesOfGroup in commit e5eb173 about HasGeneratorsOfGroup and about HasAbelianInvariants are enhanced with HasIsTrivial and HasIsPerfectGroup.

hungaborhorvath · 2016-11-10T15:22:24Z

@fingolfin Ok, I think I have managed to resolve the conflict. The master branch contained an immediate method for perfect groups, using the same lines where I added a new method for perfect groups. So I opened up the file in the editor, kept both changes, added it to the rebase, and continued the rebase, and I think now everything should work fine.

fingolfin · 2016-11-24T17:41:39Z

@hungaborhorvath Thanks for resolving the conflict. Now, I noticed that the new IsPerfectGroup method is not covered by the tests. Do you know any examples which trigger it?

hungaborhorvath mentioned this pull request Apr 2, 2016

Knowing that the group is Abelian messes up Sylow and Hall subgroup computation? #707

Closed

hungaborhorvath mentioned this pull request Apr 3, 2016

Address @hungaborhorvath's pull requests #692

Closed

6 tasks

fingolfin reviewed Nov 3, 2016

View reviewed changes

hungaborhorvath force-pushed the IsSolvableGroup branch from c4161c4 to 60e55b7 Compare November 5, 2016 23:17

fingolfin reviewed Nov 9, 2016

View reviewed changes

hungaborhorvath force-pushed the IsSolvableGroup branch from 4e61256 to 50a1023 Compare November 10, 2016 11:50

hungaborhorvath force-pushed the IsSolvableGroup branch from 50a1023 to 05c4635 Compare November 10, 2016 15:20

fingolfin merged commit 5d7cd4c into gap-system:master Dec 7, 2016

hungaborhorvath deleted the IsSolvableGroup branch December 7, 2016 15:28

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 20, 2018

Add IsAbelian filters to IsSolvableGroup and DerivedSeries #706

Add IsAbelian filters to IsSolvableGroup and DerivedSeries #706

Conversation

hungaborhorvath commented Apr 2, 2016

hungaborhorvath commented Apr 2, 2016

hungaborhorvath commented Apr 2, 2016

hulpke commented Apr 3, 2016

hungaborhorvath commented Apr 3, 2016

hulpke commented Apr 3, 2016

hungaborhorvath commented Apr 3, 2016

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

Choose a reason for hiding this comment

fingolfin commented Nov 4, 2016

fingolfin commented Nov 5, 2016

hungaborhorvath commented Nov 5, 2016

hungaborhorvath commented Nov 5, 2016

codecov-io commented Nov 5, 2016 • edited Loading

Current coverage is 48.72% (diff: 96.00%)

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

olexandr-konovalov commented Nov 9, 2016

hungaborhorvath commented Nov 10, 2016

fingolfin commented Nov 10, 2016

hungaborhorvath commented Nov 10, 2016

fingolfin commented Nov 10, 2016

hungaborhorvath commented Nov 10, 2016

fingolfin commented Nov 24, 2016

codecov-io commented Nov 5, 2016 •

edited

Loading