Maximal normal subgroups fixes #705

hungaborhorvath · 2016-04-02T09:51:30Z

Fix some issues with MaximalNormalSubgroups introduced from #692 after merging #552.

NormalSubgroup computation method is only called for finite groups.
Redispatch is added for two methods (finite, solvable). (Not added for abelian, because abelian
groups should be recognized by the IsSolvabe test.)
New generic method added checking if AbelianInvariants has 0.
Abelian method checks if AbelianInvariants has 0 (for non pc-groups).
If GAP finds that there are infinitely many maximal normal subgroups, then
it returns fail instead of erroring out (e.g. if 0 is in AbelianInvariants).
Manual is changed to reflect this new behaviour, an example is added, as well.
Lists are replaced by sorted lists in test file and some more tests are added.

I tried to heed to the guidelines set in #692. I believe there can be two possibilities for concern here. One is the obvious behaviour change if 0 is in AbelianInvariants meaning there are infinitely many maximal normal subgroups, then earlier MaximalNormalSubgroups would simply error with no method found, and now it returns fail. The other is that earlier the method calling NormalSubgroups first and then checking for maximal ones in that list is now only called for finite groups in grp.gi:4464. I do not know if any method for NormalSubgroups for infinite groups exists, but if there are some then it should be discussed how to reintroduce the computing NormalSubgroups first method for MaximalNormalSubgroups for such groups.

Test should run with -A -m 100m -o 1g, but instead of 1.8 seconds it runs for 75.5 seconds on my machine with these parameters. I am not sure if anything should be done about this.

markuspf · 2016-04-02T17:38:47Z

I am not sure whether fail is the correct response here.
If there are infinitely many maximal normal subgroups, is there hope of describing them by using an Iterator for example (and if so, will that be of any use at all?)

hungaborhorvath · 2016-04-02T18:31:13Z

@markuspf Hm, yes, they should be able to be described in such a way, at least if the group is solvable. I doubt, though, that it is worth the effort.

Originally, I made the method error out with a message in such a situation, because I assumed that the user could benefit from the knowledge of too many maximal normal subgroups, and I did not think of iterators. I still think that until a method for such an iterator is not present, it would be worth to notify the user about the situation either by erroring or failing or maybe some other way.

markuspf · 2016-04-08T11:12:22Z

lib/grp.gi

+##  can be computed very quickly. Thus if you suspect your group to be
+##  abelian or solvable, then check it before computing the maximal normal
+##  subgroups.
+##


This is not in the documentation, and I wonder whether this would be appropriate (actually both points: If you can get your hands on a character table, or if the group is abelian or solvable MaximalNormalSubgroups can be computed more effciently)?

Added the note to the manual and added an infinite example to emphasize that MaximalNormalSubgroups works for some infinite groups.

markuspf · 2016-04-11T12:25:27Z

A note on the test runtime as well. As @hulpke pointed out in #722 (and it has been noted before), the testinstall runtimes seem to be getting a bit out of hand.

I actually made a proposal for rearranging the tests a bit, and give the testsuites slightly better names: https://github.com/gap-system/gap/wiki/Testing-methods-and-targets An appropriate pull-request is in progress, but not done yet. The current testinstall target can be roughly translated to test-core-quick.

fingolfin

Thanks for your effort, and sorry for no feedback in half a year :-(.

In case you are not sick of us and still willing to work on this, perhaps you could also rebase it -- this then should trigger the new code coverage tests, which will help us see if the new code is fully exercised by the new tests.

Thanks once more!

fingolfin · 2016-11-03T13:35:54Z

lib/grp.gi

+
+    if 0 in AbelianInvariants(G) then
+      # (p) is a maximal normal subgroup in Z for every prime p
+      return fail;


Regarding this line, @markuspf previously asked whether returning fail here makes sense or not. And @hungaborhorvath stated that he initially had an Error() here, but found fail to be more useful. I mostly agree with the latter. And regarding the possibility of returning an infinite iterator here: Sure, that might be possible for some cases at least, but I also doubt it'd be that useful... Perhaps some day somebody will arrive at a situation where it is useful, and then we can revisit this. Until then, I think it would be a waste of time to implement such an iterator, plus, let's not forgot the golden rule "code you don't actually use is always wrong" ;-).

We could of course add a new constant UnableToCompute or ResultIsInfinite or so, but again: as long as nobody actually uses this...

Finally, the "rule" to a degree even applies to fail. @hungaborhorvath do you use this, or is this just a case of "might be useful"? In the latter case, I'd just put an error here. Then, if we later find a use case where we should return fail and/or some other special constant, we can reopen this, without breaking any existing code. While if we return fail now, we can never again change it without risking to break somebody's code... So in this sense, Error is the safe and future proof solution :-)

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

lib/grp.gd

+##  gap> List(MaximalNormalSubgroups(f/[x^2, y^2]), GeneratorsOfGroup);
+##  [ [ x, y*x*y^-1 ], [ y, x*y*x^-1 ], [ y*x^-1 ] ]
+##  gap> MaximalNormalSubgroups( AbelianGroup( [ 0 ] ) );
+##  fail
 ##  ]]></Example>


These documentation changes look good to me.

fingolfin · 2016-11-03T13:36:30Z

lib/grp.gi

 InstallMethod( MaximalNormalSubgroups,
    "generic search",
-    [ IsGroup ],
+    [ IsGroup and IsFinite ],


fingolfin · 2016-11-03T13:36:35Z

lib/grp.gi

+RedispatchOnCondition( MaximalNormalSubgroups, true,
+    [ IsGroup ],
+    [ IsFinite ], 0);
+


fingolfin · 2016-11-03T13:37:17Z

lib/grp.gi

-  RankFilter(IsGroup and IsFinite)
-  - RankFilter(IsGroup and IsNilpotentGroup),
+  RankFilter( IsGroup and IsFinite and IsNilpotentGroup )
+  - RankFilter( IsGroup and IsNilpotentGroup ),


makes sense

fingolfin · 2016-11-03T13:51:12Z

lib/grppcatr.gi

+            # convert it to an Abelian PcGroup with same invariants
+            Gf := AbelianGroup(IsPcGroup, AbInv);
+            hom := IsomorphismGroups(G, Gf);
+            MaxGf := NormalMaximalSubgroups(Gf);


perhaps add a comment which points out that for abelian groups, all maximal normal subgroup are also normal maximal subgroups (and vice-versa), lest people might get confused (well... "people" = me in this case... but perhaps I am unique in this regard ... :-).

fingolfin · 2016-11-03T13:51:54Z

lib/grppcatr.gi

+        else
+            # convert it to an Abelian PcGroup with same invariants
+            Gf := AbelianGroup(IsPcGroup, AbInv);
+            hom := IsomorphismGroups(G, Gf);


Computing IsomorphismGroups might be wasteful here... Why not use IsomorphismPcGroup? I.e. replace the preceding two lines by

hom := IsomorphismPcGroup(G); Gf := Image(hom);

As a bonus, if G is already a pc group, then this immediatly returns an identity mapping.

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

lib/grppcatr.gi

+               # rank, otherwise the method for normal subgroup computation
+               # is selected.
+               RankFilter( IsGroup and IsFinite and IsSolvableGroup )
+               - RankFilter( IsGroup and IsSolvableGroup ),


fingolfin · 2016-11-03T13:52:43Z

lib/grppcatr.gi

+RedispatchOnCondition( MaximalNormalSubgroups, true,
+    [ IsGroup ],
+    [ IsSolvableGroup ], 0);
+


fingolfin · 2016-11-03T13:54:34Z

tst/testinstall/opers/MaximalNormalSubgroups.tst

-  [ 2, 4, 5, 7, 8, 9, 25 ], [ 2, 4, 5, 7, 8, 9, 25 ], 
-  [ 2, 4, 5, 7, 8, 9, 25 ], [ 2, 3, 4, 5, 5, 7, 8, 9 ], 
+gap> SortedList(List(MaximalNormalSubgroups(A),N -> AbelianInvariants(N)));
+[ [ 2, 2, 3, 5, 7, 8, 9, 25 ], [ 2, 2, 3, 5, 7, 8, 9, 25 ], 


Yup, using SortedList seems like a good idea, to keep the tests stable

hungaborhorvath · 2016-11-04T20:43:46Z

@fingolfin I think I have managed to remove the commit about the fail (originally it was error, so that should do the trick). In the last commit I have applied your suggestions, I hope I did not leave out anything. Also rebased, hopefully succeeded.

Edit: Nope, I must have messed up something.... I will take it slowly and one step at a time.

Edit2: It should be ok now. Let us see if tests run.

codecov-io · 2016-11-04T21:23:11Z

Current coverage is 48.66% (diff: 92.30%)

Merging #705 into master will decrease coverage by 0.15%

@@             master       #705   diff @@
==========================================
  Files           424        424          
  Lines        222089     222098     +9   
  Methods        3426       3426          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         108441     108091   -350   
- Misses       113648     114007   +359   
  Partials          0          0

Powered by Codecov. Last update becaeff...d4181c3

- NormalSubgroup computation method is only called for finite groups. - Redispatch is added for two methods (finite, solvable). - New generic method added checking if AbelianInvariants has 0. - Abelian method checks if AbelianInvariants has 0 (for non pc-groups). - Lists are replaced by sorted lists in test file.

fingolfin · 2016-11-05T14:45:03Z

lib/grp.gi

+
+    if 0 in AbelianInvariants(G) then
+      # (p) is a maximal normal subgroup in Z for every prime p
+      Error("number of maximal normal subgroups is infinity");


According to https://codecov.io/gh/gap-system/gap/pull/705/compare this is not covered by the tests. Could you please add a test case? E.g. using an abelian FP group with a single generator)

Actually, you might need one with two generators, as one with a single generator is abelian, the systems knows that, and thus the other currently untested error should get triggered... :)

@fingolfin Previously, when it returned fail, I had a test for it, that is not a problem.

But I have no idea how to write a test for an error into a tst file. Should I just copy everything from my GAP output to the tst file? Or does it have a special method? Is the output even stable (apart from the error message)?

@hungaborhorvath : I often find the easiest thing to do is to run the test without an output, and then cut+paste the output GAP gives you (when it tells you the test is wrong). Here's an example test which produces an error.

gap> 1/0; Error, Rational operations: <divisor> must not be zero

Note how the line / file / backtrace are all missing (they are disabled when you test, specifically for this reason).

The .tst file basically should contain the first line of the error message.. but as @ChrisJefferson says, you can simply run the test file manually, and if you got it wrong, it'll tell you.

Just in case you don't know, you can run an individual test file with the Test functio (that's much quicker than running the whole make testinstall etc.)

fingolfin · 2016-11-05T14:47:12Z

lib/grppcatr.gi

+        AbInv := AbelianInvariants(G);
+        if 0 in AbInv then
+            # (p) is a maximal normal subgroup in Z for every prime p
+            Error("number of maximal normal subgroups is infinity");


This line is untested, too.

hungaborhorvath · 2016-11-05T17:21:52Z

I think I have added tests for the errors. Let me know if I missed any, I am not sure how to read codecov.... Thank you.

fingolfin · 2016-11-06T12:15:31Z

To read codecov, click on the "Current coverage" link, then on the webpage that opens, there is a horizontal list ("tab bar"?) with four enries: Overview, Commits, Compare, Graphs. Select "compare". The resulting page shows you the changes in this PR; light red and light green to indicate removals / additions. Now, in the left most column (the one with line numbers) you'll also see dark green / dark red blotches. Those indicate lines that are actually code; green means this line is executed by some test, while dark red means this line is never triggered. Right now, everything is reached, except for one "end" line, which is kind of a bug, I think (well, it indeed is never reached, because we return before it... perhaps moving the TryNextMethod() out of the else and to the top level of the function helps? In any case, it is is not important, your test coverage is perfect.).

fingolfin · 2016-11-06T12:15:47Z

@ChrisJefferson would know better regarding that red "end" ... but again, it's harmless

fingolfin · 2016-11-06T12:18:11Z

Does the test still require 75 seconds, instead of 2? That is something we need to worry about. Perhaps we can split the test in those which are run on testinstall, and those which are run in the full test suite... or perhaps we can replace some of the slow tests by faster ones? Haven't yet had a chance to figure out which is causing what.

hungaborhorvath · 2016-11-06T12:44:38Z

@fingolfin Thank you for explaining the codecov, I understand it now.

About the running time: yes, without packages it is still very long. I guess it is the fp-groups, but I have not checked explicitly.

ChrisJefferson · 2016-11-06T12:54:06Z

The red 'end' is because GAP adds an implicit "return" to the end of functions, which is then not executed. I am currently doing a cleanup which will clean up this, and some other issues.

hungaborhorvath · 2016-11-06T13:45:45Z

The culprit for the long runtime was the fp-group D360. I changed to D36, and now tests run in less than 1 second with or without packages. I do not remember why I chose 360 originally....

fingolfin · 2016-11-09T12:22:42Z

For me, the tests now indeed run in less than 1 second. While on master (!), it takes over 70 seconds to run tst/testinstall/opers/MaximalNormalSubgroups.tst.

Clear win! :-)

markuspf · 2016-11-09T12:25:38Z

Thank you very much for your hard work @hungaborhorvath!

hungaborhorvath mentioned this pull request Apr 3, 2016

Address @hungaborhorvath's pull requests #692

Closed

6 tasks

markuspf reviewed Apr 8, 2016
View reviewed changes

fingolfin reviewed Nov 3, 2016

View reviewed changes

hungaborhorvath force-pushed the MaximalNormalSubgroupsFixes branch from 5b8dd6c to 562925f Compare November 4, 2016 20:41

hungaborhorvath force-pushed the MaximalNormalSubgroupsFixes branch from 562925f to 5b8dd6c Compare November 4, 2016 22:15

hungaborhorvath force-pushed the MaximalNormalSubgroupsFixes branch from 5b8dd6c to 96da71e Compare November 4, 2016 22:18

Add efficiency note and infinite example to MaximalNormalSubgroups

19a57d3

hungaborhorvath force-pushed the MaximalNormalSubgroupsFixes branch from 96da71e to 19a57d3 Compare November 4, 2016 22:43

hungaborhorvath added 2 commits November 4, 2016 23:58

Apply Max Horn's suggestions on some comments

279c05e

Replace IsomorphismGroup by IsomorphismPcGroup

6a325c0

hungaborhorvath force-pushed the MaximalNormalSubgroupsFixes branch from 118c73f to 6a325c0 Compare November 4, 2016 23:00

Rephrase documentation on infinite maximal subgroups

3b7d65d

fingolfin reviewed Nov 5, 2016

View reviewed changes

Add tests for infinite maximal normal subgroups

f688870

Modify test to run faster

d4181c3

fingolfin merged commit 312ab24 into gap-system:master Nov 9, 2016

hungaborhorvath deleted the MaximalNormalSubgroupsFixes branch November 9, 2016 19:02

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

Maximal normal subgroups fixes #705

Maximal normal subgroups fixes #705

Conversation

hungaborhorvath commented Apr 2, 2016 • edited by fingolfin Loading

markuspf commented Apr 2, 2016

hungaborhorvath commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Apr 11, 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

hungaborhorvath commented Nov 4, 2016 • edited Loading

codecov-io commented Nov 4, 2016 • edited Loading

Current coverage is 48.66% (diff: 92.30%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson Nov 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hungaborhorvath commented Nov 5, 2016

fingolfin commented Nov 6, 2016

fingolfin commented Nov 6, 2016

fingolfin commented Nov 6, 2016

hungaborhorvath commented Nov 6, 2016

ChrisJefferson commented Nov 6, 2016

hungaborhorvath commented Nov 6, 2016

fingolfin commented Nov 9, 2016

markuspf commented Nov 9, 2016

hungaborhorvath commented Apr 2, 2016 •

edited by fingolfin

Loading

hungaborhorvath commented Nov 4, 2016 •

edited

Loading

codecov-io commented Nov 4, 2016 •

edited

Loading

ChrisJefferson Nov 5, 2016 •

edited

Loading