Fix rankings for Socle and MinimalNormalSubgroups #711

hungaborhorvath · 2016-04-03T15:07:13Z

Fixed the rankings of Socle and MinimalNormalSubgroups methods for nilpotent groups.

Some old methods for Socle or for MinimalNormalSubgroups were called for arbitrary groups, but they seem to only work for finite groups, and thus their filters have been changed. Further, for two methods I put in a forced IsNilpotent check, because for such groups the nilpotent method seems to be much more faster.

Added some new tests. All tests run without packages, as well. Interestingly, they are much faster without packages (408ms vs 644ms for Socle.tst, 1884ms vs 2332ms for MinimalNormalSubgroups.tst). The reason is that CRISP is rather aggressively checks for solvability and finiteness (in fact, there is a CRISP_RedispatchOnCondition command, which I guess does redispatch even if there were other applicable methods with lower ranks. And solvable methods are slower than nilpotent methods, thus the speed difference.

Note that there are no explicit methods for solvable groups without CRISP, thus I never forced an IsSolvableGroup check in the low ranked methods.

hulpke · 2016-04-03T15:10:02Z

lib/grplatt.gi

+  # should have and IsSolvable check, as well,
+  # but methods for solvable groups are only in CRISP
+  # which aggeressively checks for solvability, anyway
+  if IsNilpotentGroup(G) then


This will run into an infinite loop if for some reason this method gets selected by the method selection for a nilpotent group. One should use
not HasNilpotentGroup(G) and IsNilpotentGroup(G).

No, because the nilpotent method is ranked higher. I first tried exiting with TryNextMethod, but that did not work, and then I checked the manual and this is the way it is supposed to be used.

This is assuming that the methods provided are static. One could imagine that the current method for nilpotent groups goes away at some point or for some strange reason becomes ranked lower. Then the problem would crop up. (It is not likely, but neither is the extra test hard.)

Sorry, I did not see your suggestion about checking for HasIsNilpotentGroup first. Applied it.

Sorry -- the suggestion wasn't there first place and then I realized the fix might not be obvious and added it. Thanks for changing it.

fingolfin

Looks good overall to me. Could you perhaps rebase it on master, as with your other PRs?

fingolfin · 2016-11-04T22:17:41Z

lib/grp.gi

+  if (not HasIsNilpotentGroup(G) and IsNilpotentGroup(G)) then
+      return MinimalNormalSubgroups( G );
+    fi;
+


Something is wrong with the indentation here (tabs vs spaces perhaps?)

fingolfin · 2016-11-04T22:18:55Z

lib/grp.gi

@@ -1693,7 +1693,8 @@ InstallMethod( Socle, "for elementary abelian groups",
 ##
 InstallMethod( Socle, "for nilpotent groups",
              [ IsGroup and IsNilpotentGroup ],
-              SUM_FLAGS,
+              RankFilter( IsGroup and IsFinite and IsNilpotentGroup )
+              - RankFilter( IsGroup and IsNilpotentGroup ),
  function(G)


Looks reasonable.

Of course this constitutes a rather large drop in rank for that method, so there could be some regressions. Alas, I don't think that's a reason to block this patch -- we'll just have to watch out for regressions, and should there be any, fix them.

fingolfin · 2016-11-04T22:21:11Z

lib/grplatt.gi

@@ -2232,11 +2232,20 @@ InstallMethod(NormalSubgroups,"homomorphism principle perm groups",true,
 ##
 #M  Socle(<G>)
 ##
-InstallMethod(Socle,"from normal subgroups",true,[IsGroup],0,
+InstallMethod(Socle,"from normal subgroups",true,[IsGroup and IsFinite],0,
 function(G)
 local n,i,s;
  if Size(G)=1 then return G;fi;


I know this is not part of your patch, but I wonder whether we should replace Size(G)=1 by IsTrivial(G) here. It should never be worse, but in theory, it can handle some cases efficiently which Size(G)=1 can not. Then again, those cases are mostly cases where G is huge, and for those cases, we plan on doing more complicated stuff below anyway...

So, probably don't do anyhing, and ignore me thinking out loud... :)

fingolfin · 2016-11-04T22:22:26Z

tst/testinstall/opers/Socle.tst

+true
+gap> G := SmallGroup(24,12);;
+gap> IdGroup(Socle(G));
+[ 4, 2 ]
 gap> A := DihedralGroup(16);;


Could you briefly elaborate why this particular tests were added?

Hm, I do not remember exactly, but probably to increase code coverage.... Rebase and indentation should be fixed.

Also fix an indentation.

codecov-io · 2016-11-05T00:11:03Z

Current coverage is 48.88% (diff: 66.66%)

Merging #711 into master will increase coverage by 0.05%

@@             master       #711   diff @@
==========================================
  Files           424        424          
  Lines        222089     222095     +6   
  Methods        3426       3426          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         108441     108563   +122   
+ Misses       113648     113532   -116   
  Partials          0          0

Powered by Codecov. Last update becaeff...3602058

fingolfin · 2016-11-05T14:42:20Z

lib/grplatt.gi

+  # but methods for solvable groups are only in CRISP
+  # which aggeressively checks for solvability, anyway
+  if (not HasIsNilpotentGroup(G) and IsNilpotentGroup(G)) then
+    return Socle(G);


This line is not covered by the tests according to https://codecov.io/gh/gap-system/gap/pull/711/compare

fingolfin · 2016-11-05T14:42:56Z

lib/grp.gi

+    # but methods for solvable groups are only in CRISP
+    # which aggeressively checks for solvability, anyway
+    if (not HasIsNilpotentGroup(G) and IsNilpotentGroup(G)) then
+      return MinimalNormalSubgroups( G );


This line is not covered by the tests according to https://codecov.io/gh/gap-system/gap/pull/711/compare

@fingolfin Actually, these two lines will never run if CRISP is loaded, because CRISP has a redispatch for finiteness and solvability checking with a higher rank, and then the CRISP method will be chosen. Nevertheless, I added a test for Socle and one for MinimalNormalSubgroups, and they are supposed to trigger the codelines in question if CRISP is not loaded.

Fixed the rankings of Socle and MinimalNormalSubgroups methods for nilpotent groups. Some old methods for Socle or for MinimalNormalSubgroups were called for arbitrary groups, but they seem to only work for finite groups, and thus their filters have been changed. Further, for two methods we now force an IsNilpotent check, because for such groups the nilpotent method seems to be much more faster. Added some new tests. All tests run without packages, as well. Interestingly, they are much faster without packages. The reason is that CRISP is rather aggressively checks for solvability and finiteness (in fact, there is a `CRISP_RedispatchOnCondition` command, which seems to redispatch even if there were other applicable methods with lower ranks. And solvable methods are slower than nilpotent methods, thus the speed difference. Note that there are no explicit methods for solvable groups without CRISP, thus we never force an `IsSolvableGroup` check in the low ranked methods.

hulpke reviewed Apr 3, 2016
View reviewed changes

hungaborhorvath mentioned this pull request Apr 3, 2016

Address @hungaborhorvath's pull requests #692

Closed

6 tasks

fingolfin approved these changes Nov 4, 2016

View reviewed changes

hungaborhorvath added 2 commits November 4, 2016 23:29

Fix rankings for Socle and MinimalNormalSubgroups

ca04199

Apply Hulpke's suggestion on checking for nilpotency carefully

bc47381

Also fix an indentation.

hungaborhorvath force-pushed the MinimalNormalSubgroupsFixes branch from 328269a to bc47381 Compare November 4, 2016 22:33

fingolfin reviewed Nov 5, 2016

View reviewed changes

Add two more tests to increase code coverage if no packages are loaded

3602058

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

hungaborhorvath deleted the MinimalNormalSubgroupsFixes 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: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rankings for Socle and MinimalNormalSubgroups #711

Fix rankings for Socle and MinimalNormalSubgroups #711

hungaborhorvath commented Apr 3, 2016

hulpke Apr 3, 2016

hungaborhorvath Apr 3, 2016

hulpke Apr 3, 2016

hungaborhorvath Apr 3, 2016

hulpke Apr 3, 2016

fingolfin left a comment

fingolfin Nov 4, 2016

fingolfin Nov 4, 2016

fingolfin Nov 4, 2016

hungaborhorvath Nov 4, 2016

fingolfin Nov 4, 2016

hungaborhorvath Nov 4, 2016

codecov-io commented Nov 5, 2016 •

edited

Loading

fingolfin Nov 5, 2016

fingolfin Nov 5, 2016

hungaborhorvath Nov 5, 2016

Fix rankings for Socle and MinimalNormalSubgroups #711

Fix rankings for Socle and MinimalNormalSubgroups #711

Conversation

hungaborhorvath commented Apr 3, 2016

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

codecov-io commented Nov 5, 2016 • edited Loading

Current coverage is 48.88% (diff: 66.66%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 5, 2016 •

edited

Loading