FIX: (Maximal) subgroups computation. #2488

hulpke · 2018-05-24T17:55:52Z

The routine for `MaximalSubgroupClassReps allows options to calculate
maximal subgroups only up to specified limits, or if it is not too
difficult. This causes problems is such a partial list is stored as
attribute. Thus separate into two attributes: One to calculate the
guaranteed full list, one to calculate a potentially partial list subject to
restrictions. Also make sure that limiting options do not get accidentally
inherited.
Finally allow a soft fallback to old intermediate subgroup routines, if
maximal subgroups are not available. (This can go away once proper maximal
subgroups code is available.)

This fixes a bug reported (Subject:Intermediate subgroup-Gap 4.9.1) on the support email list.

Included is also a fix for the main bug in #2586

Also added test files.

In the best of all worlds this should be backported to 4.9

fingolfin · 2018-05-24T19:38:11Z

lib/grp.gd

@@ -1202,7 +1202,11 @@ DeclareAttribute( "MaximalSubgroups", IsGroup );
 ##
 DeclareAttribute("MaximalSubgroupClassReps",IsGroup);

+# utility attribute: Allow use with limiting options, so could hold `fail'.
+DeclareAttribute("TryMaximalSubgroupClassReps",IsGroup);


What I don't understand about this is why TryMaximalSubgroupClassReps even is an attribute, and not just an operation, as its value may depend on options, and there is no way for which options the set attribute value was computed, is there?

(Perhaps also a mutable attribute would be a choice, so I don't quite see why/how)?

Mind you, I am not saying this is wrong, just that I don't understand the rationale, and would appreciate some illuminating words. Thanks!

@fingolfin
I'm still trying to get the tests run through, so it might be worth to wait with reviewing until the checks show green.

The reason for making Try... an attribute is that a calculation might (in fact that happens when calculating double cosets) several times ask for cheap maximal subgroups of the same group.

codecov · 2018-05-24T20:16:09Z

Codecov Report

Merging #2488 into master will increase coverage by 0.63%.
The diff coverage is 51.31%.

@@            Coverage Diff             @@
##           master    #2488      +/-   ##
==========================================
+ Coverage   74.71%   75.34%   +0.63%     
==========================================
  Files         480      443      -37     
  Lines      242188   228948   -13240     
==========================================
- Hits       180945   172504    -8441     
+ Misses      61243    56444    -4799

Impacted Files	Coverage Δ
lib/gpprmsya.gi	`66.96% <ø> (-0.67%)`	⬇️
lib/factgrp.gi	`82.43% <0%> (ø)`	⬆️
lib/grpnice.gi	`81.64% <0%> (-2.85%)`	⬇️
lib/pcgsperm.gi	`85.85% <0%> (-0.11%)`	⬇️
lib/grppcatr.gi	`80.08% <0%> (-0.18%)`	⬇️
lib/maxsub.gi	`63.05% <100%> (+0.05%)`	⬆️
lib/grp.gi	`84.98% <16%> (-0.66%)`	⬇️
lib/grplatt.gi	`69.99% <84.61%> (+7.89%)`	⬆️
lib/csetgrp.gi	`62.13% <92%> (+3.35%)`	⬆️
lib/methsel2.g	`34.84% <0%> (-9.1%)`	⬇️
... and 80 more

hulpke · 2018-05-27T12:52:00Z

@fingolfin
OK, it now has run through and I have rebased away patches. It affects a comparatively large number of files, but basically because the use of options in the interface was badly designed. As far as the functionality is affected it is actually a quite local patch at the interface of Maximal subgroups and intermediate subgroups.

[Moved comment out of `outdated' bit.

fingolfin · 2018-05-27T22:41:56Z

lib/grp.gi

+
+end);
+
+InstallGlobalFunction(TryMaxSubgroupTainter,function(G)


Hmm... This whole maxsubtrytaint approach seems brittle to me. Forget a call to TryMaxSubgroupTainter in one place, and boom, things go wrong, possibly in a hard to track way.

Instead of using an approach were a failure to comply with the (undocumented!) leads to mathematically wrong results, I'd prefer if we could use one were failure to comply just leads to a slow down.

E.g. instead of requiring every TryMaximalSubgroupClassReps method to invoke TryMaxSubgroupTainter, let's instead require them to invoke Set(MaxSubgroupTainter before returning, if applicable.

Things get even simpler if you also add an operation TryMaximalSubgroupClassRepsOp, and move most current MaximalSubgroupClassReps resp. TryMaximalSubgroupClassReps methods to that operation. Then, both MaximalSubgroupClassReps resp. TryMaximalSubgroupClassReps can dispatch to that operation. This would also remove the need to create a temporary "copy" of the group, and later transfer subgroups back to the original group.

And the main (only?) TryMaximalSubgroupClassReps method can look roughly like this:

function(G) local maxs; if HasMaximalSubgroupClassReps(G) then return MaximalSubgroupClassReps(G); fi; maxs := TryMaximalSubgroupClassRepsOp(G); if NO-BAD-OPTIONS-SET then SetMaximalSubgroupClassReps(G, maxs); fi; return maxs` end;

That's still slightly brittle, because some TryMaximalSubgroupClassRepsOp might accept more options in the future, and if we forget to update the above method, we have a problem... But at least we won't be off worse than we are now.

(BTW, I'd actually turn those options into explicit arguments to TryMaximalSubgroupClassRepsOp, but that's probably a matter of taste)

PS: of course one could also try to come up with a better name... perhaps the second attribute should be SomeMaximalSubgroupClassReps, and the operation then could be TryMaximalSubgroupClassReps, or ComputeSomeMaximalSubgroupClassReps, or ... but that's of course in the end irrelevant, the functionality is what counts.

fingolfin · 2018-05-27T22:47:14Z

lib/grp.gi

+  # compute anew for new group to avoid taint
+  H:=Group(GeneratorsOfGroup(G));
+  for i in [Size,IsNaturalAlternatingGroup,IsNaturalSymmetricGroup] do
+    if Tester(i)(G) then Setter(i)(H,i(G));fi;


So how do we ensure that these three attributes are the only relevant ones for computations of maximal subgroups? What about e.g. IsSolvableGroup? Or information about stabilizer chains, etc. etc.?

They are currently the only two for which special methods for maximal subgroups are installed.

fingolfin · 2018-05-27T22:49:42Z

lib/csetgrp.gi

-          return c^k;
+  # use maximals, use `Try` as we call with limiting options
+  IsNaturalAlternatingGroup(G);
+  IsNaturalSymmetricGroup(G);


The above two lines are new. I assume they are meant to ensure better methods are applied, if possible? I.e. an optimization, but not a vital part of the fix? (Just trying to understand).

Yes. But this optimization is checked for in manual and test examples.

fingolfin · 2018-05-27T22:52:30Z

lib/csetgrp.gi

+  IsNaturalAlternatingGroup(G);
+  IsNaturalSymmetricGroup(G);
+  m:=TryMaximalSubgroupClassReps(G:cheap,intersize:=intersize,nolattice);
+  if m<>fail and Length(m)>0 then


Was the nolattice option added here on purpose? Just wondering, as I don't see this part of the change mentioned in the commit message.

fingolfin · 2018-05-27T22:59:18Z

tst/testbugfix/2018-05-24-IntermediateSubgroups.tst

+# observation by S.Alavi with IntermediateGroup
+
+gap> g:= AtlasGroup( "U4(4)" );;  Size( g );
+1018368000


The atlasrep package may not be available (or at least not loaded) when running the tests. Could this perhaps be changed to g:=SU(IsPermGroup,4,4);?

fingolfin · 2018-05-27T23:22:24Z

tst/testbugfix/2018-05-24-IntermediateSubgroups.tst

+gap> g:= AtlasGroup( "U4(4)" );;  Size( g );
+1018368000
+gap> h:= Image( IsomorphismPermGroup( PSL(2,16) ) );;
+gap> l:= IsomorphicSubgroups( g, h );;


This computation can take minutes when I try it here. Since only one of those groups is needed, can't we just explicitly describe it? Or, just switch to a smaller example, like this (I verified that it fails in master, too):

gap> g:=SU(IsPermGroup,3,4); Perm_SU(3,4) gap> h:=PSL(IsPermGroup, 2, 4);; gap> l:= IsomorphicSubgroups( g, h );; Length(l); 1 gap> s1:= Image( l[1] );; Size( s1 ); 60 gap> n1:= Normalizer( g, s1 );; Size( n1 ); 300 gap> int:=IntermediateGroup(g,s1);; gap> IsGroup(int); true

That way, the bugfix test suite does not get slowed down, nor does it require the atlasrep package.

Yes, the IsomorphicSubgroups can go and has done so. The issue of the bug was that it was a (comparatively) small index subgroup in a large group, so large indeed that it is unlikely to be added to the table of marks library anytime soon.

My personal inclination is at this time to have the original bug, but if someone wants to change it afterwards for speed reasons I have no objection.

lib/grp.gi

@@ -1342,6 +1342,50 @@ end);
 ##
 #M  MaximalSubgroups( <G> )
 ##
+InstallMethod(MaximalSubgroupClassReps,"default, catch dangerous options",


fingolfin · 2018-05-27T23:29:08Z

lib/grp.gi

+
+  # now we know list is untained, store 
+  Unbind(G!.maxsubtrytaint);
+  G!.TryMaximalSubgroupClassReps:=l;


So you bypass the setter function. Any particular reason for that? It also means that Has TryMaximalSubgroupClassReps won't be set.

In this situation the attribute is already set with a list that is not guaranteed to be complete. We also have just computed a complete list and we store it in place. Can I use a setter to change the value?

For mutable attributes: yes!

gap> G:=SymmetricGroup(5); Sym( [ 1 .. 5 ] ) gap> SetIsFoo(G, "foo"); gap> IsFoo(G); "foo" gap> SetIsFoo(G, "nar"); gap> IsFoo(G); "nar"

hulpke · 2018-06-15T08:56:26Z

As far as I'm concerned this fix is complete. If someone wants to make additions to it after merging (or propose a different fix) feel free to do so. I am happy to help with integrating it into 4.9, but not so much in discussing possible other code structures or function names.

I have included the suggestions that to me made sense as part of a fix. I have not done code restructuring or manual additions. Let me explain why:

This is a fix that should be back ported to 4.9. For this it is unfortunate that it requires a new attribute, but still, the changes ought to be as small as possible. This is the reason for not doing structural changes.

As for undocumented options, these serve two purposes:

Stop an infinite recursion (because in some cases it is helpful to have maximal subgroups of smaller groups)
Work around the real issue, namely GAP's lack of maximal subgroup information for classical (or even Lie-Type) groups. This means that a maximal subgroup calculation might need to fall back on expensive subgroup lattice computations, this needs to be stopped if maximal subgroups are only used to get some proper subgroups cheap and quickly.

In the end, I hope we will have (as a port of the Magma functionality that I am working on) this lacking functionality and at that point both the Try... attribute (which is there to store down incomplete information) as well as most of the options can go.
If we document either, they might get used by other code, and then are hard to get rid off again.

I am also happy to look, for future releases, at changes for the attribute to clean up the quick'' versus complete'' situation without abusing attributes.
This however will likely introduce some syntax incompatibilities and thus is not appropriate for a bugfix.

fingolfin

While I still am not really happy with TryMaxSubgroupTainter (because I think it's very easy to produce incorrect code with this approach; while the alternative I suggested should not require more changes, in fact, I think it needs fewer), I also appreciate that reworking this requires time, effort and nerves, and this fix is definitely much better than no fix! We can indeed (if we want to) improve on it later.

I am slightly unsure about backporting this to GAP 4.9, given the magnitude of the change. But if others are fine with it... @alex-konovalov could this still make GAP 4.9.2 (I am not quite sure about the release schedule for that).

BTW, note that our plan is that GAP 4.10 will branch around September 1st, and 4.10.0 be released around October 1st (whether this will be already public, or again "just" a beta as before, with the first public release 4.10.1. on 1st of November, still needs to be discussed.

For record, the cryst package also installs two methods for MaximalSubgroupClassReps which are checking various options (though I have not looked further into it, i.e. I don't know if these options affect the result). So I guess once this PR is merged, we should contact @gaehler to discuss with him whether cryst needs an update, too?

fingolfin · 2018-06-28T09:18:59Z

tst/testbugfix/2018-05-24-IntermediateSubgroups.tst

+# slow otherwise. Also construct the smaller subgroup s1 directly.
+# Finally do not slow down with assertions that don't need testing here
+
+gap> SetAssertionLevel(0);;


If you turn off assertions here, this will also turn them off in subsequent tests -- until some test uses START_TEST/STOP_TEST, which not all .tst files do right now.
So please either manually save/restore the assertion level, or else (probably simpler) add START_TEST and STOP_TEST calls.

Alternatively, somebody else can add this after this PR gets merged.

fingolfin · 2018-06-28T09:34:50Z

tst/testbugfix/2018-06-28-grplatt.tst

@@ -0,0 +1,7 @@
+# #2586


If we ever switch away from GitHub, this reference will be completely inscrutable. Could you perhaps change it to e.g. this?

# Verify that ConjugacyClassesSubgroups returns classes with representatives that actually are # contained in the group (bug #2586 on GitHub)

Or if you want to stay brief (as the test actually is quite self-explanatory), perhaps at least

# verify fix for bug #2586 on GitHub

fingolfin · 2018-06-28T09:36:42Z

lib/grplatt.gi

-      len:=LengthsTom(tom);
-      sub:=List([1..Length(len)],x->RepresentativeTom(tom,x));
+	  Identifier(tom[2])," from table of marks");
+      len:=LengthsTom(tom[2]);


This looks plausible to me, thanks.

Quick remark on the commit message: It contains the string "partially fixes #2586" -- GitHub sees that, but only the "fixes #2586" bit, it doesn't care about the "partially". As a result, if this commit gets merged, GitHub will close issue #2586.

One workaround is to rephrase this to e.g. fixes issue #2586 (I think). Well, or we wait for you to also fix the second bug, then closing the issue is fine, after all ;-)

fingolfin · 2018-06-28T10:43:52Z

tst/testbugfix/2018-06-28-grplatt.tst

+gap> cl:=ConjugacyClassesSubgroups(g);;
+gap> ForAll(cl,x->IsSubset(g,Representative(x)));
+true
+


This test file actually fails the test due to this extra new line at the end. Removing it should be enough to fix the test.

markuspf

Subject to the tests passing this looks good to me.

olexandr-konovalov · 2018-06-28T22:25:17Z

About backporting - ideally, I'd like to see only the fix for #2586 backported to stable-4.9 - that is easy to do if it's in separate commit(s). But is it independent of the other changes in this branch?

The milestone for 4.9.2 has the date 8th July, and that seems realistic - I expect that the changes overview will be merged and most of the rest will be reassigned to GAP 4.9.3 (like it was reassigned to 4.9.2 earlier).

We will have PGTC 2018 (http://www-groups.mcs.st-and.ac.uk/~pgtc2018/) in St Andrews in mid-July and would like to have 4.9.2 prior to that.

The routine for `MaximalSubgroupClassReps allows options to calculate maximal subgroups only up to specified limits, or if it is not too difficult. This causes problems is such a partial list is stored as attribute. Thus separate into two attributes: One to calculate the guaranteed full list, one to calculate a potentially partial list subject to restrictions. Also make sure that limiting options do not get accidentally inherited. (In the big scheme of things we might want to revisit the question of ``cheap attributes'' more generally, as composition tree uses similar paradigms.) Finally allow a soft fallback to old intermediate subgroup routines, if maximal subgroups are not available. (This can go away once proper maximal subgroups code is available.) Also added test file In the best of all worlds this should be backported to 4.9 Reworked the example. Avoiding the AtlasGroup command makes it much harder as SU gives a group on >>325 points. Also construct the subgroup directly to avoid homomorphism search.

hulpke · 2018-06-29T09:30:21Z

The two fixes now are in two separate, non-interleaved commits.

markuspf · 2018-06-29T09:41:23Z

Is the intention for this to be merged once the tests pass, then?

hulpke · 2018-06-29T10:14:46Z

@alex-konovalov
As for back porting, this is of course your decision.

Just in case this is relevant information, this first fix (for MaximalSubgroups) resolves a bug reported by S. Alavi from Iran on 5/24 (subject Intermediate subgroup-Gap 4.9.1).
That report BTW never got an official reply.

This bug likely did not show up earlier since the TOM library was not loaded by default. Even an example is provided, the latter thrice as error does not arise every time. This fixes gap-system#2586

fingolfin · 2018-06-29T14:12:20Z

Re: the bug report by S. Alavi not having been replied to: that's of course unfortunate. But it's the usual problem when people start replying to the report, but not to the reporter, and then nobody remembers to reply with at least an "we are looking into it".
So, to make up for that, once this is merged, "somebody" (who?) should reply to S. Alavi after all, and confirm the issue, and indicate the upcoming bug fix.

fingolfin · 2018-06-29T14:13:34Z

As to backporting: I think only the first commit can and should be backported -- the second fixes a regression that does not affect stable-4.9, after all (and modifies code that's not even on that branch, so it wouldn't even apply).

fingolfin · 2018-06-29T14:17:19Z

I'll reply to S. Alavi

hulpke · 2018-06-29T15:40:07Z

@fingolfin
Thank you very much for replying to Alavi.

gaehler · 2018-07-02T12:49:52Z

I still do not understand what the impact of this change on the cryst package is. According to the manual, attribute values computed from a call with more than one argument are never stored. Is this still true? If so, no changes to cryst are necessary. Otherwise, I do have a problem...

For affine crystallographic groups, MaximalSubgroupClassReps must always be called with two arguments, because the total number of classes is generally not finite. This is checked, and an error is returned otherwise. I hope this will just continue to work, without using TryMaximalSubgroupClassReps.

gaehler · 2018-07-02T13:22:29Z

I have to correct myself. A method for one argument is actually missing, and the default one won't work unless the group is finite. Maybe I should just add a method that catches the finite case, and returns an error with explanation otherwise.

hulpke added kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them backport-to-4.9 labels May 24, 2018

fingolfin reviewed May 24, 2018

View reviewed changes

hulpke force-pushed the ah/maxsub branch from 57632d0 to 3fa9038 Compare May 25, 2018 14:56

fingolfin reviewed May 27, 2018

View reviewed changes

fingolfin added the status: awaiting response Issues and PRs whose progress is stalled awaiting a response from (usually) the author label Jun 11, 2018

hulpke force-pushed the ah/maxsub branch from 3fa9038 to 9e97a66 Compare June 15, 2018 08:16

hulpke removed the status: awaiting response Issues and PRs whose progress is stalled awaiting a response from (usually) the author label Jun 15, 2018

hulpke force-pushed the ah/maxsub branch from 9e97a66 to 5e8d22d Compare June 15, 2018 08:22

hulpke force-pushed the ah/maxsub branch from 5e8d22d to 6068414 Compare June 19, 2018 14:15

hulpke mentioned this pull request Jun 28, 2018

Invalid result of ConjugacyClassesSubgroups(TransitiveGroup(6,14)); if "tomlib" and "atlasrep" are loaded #2586

Closed

hulpke force-pushed the ah/maxsub branch from 9b091be to 6780793 Compare June 28, 2018 09:25

hulpke changed the title ~~FIX: MaximalSubgroupClassReps with options.~~ FIX: (Maximal) subgroups computation. Jun 28, 2018

fingolfin approved these changes Jun 28, 2018

View reviewed changes

fingolfin reviewed Jun 28, 2018

View reviewed changes

olexandr-konovalov added this to the GAP 4.9.2 milestone Jun 28, 2018

markuspf approved these changes Jun 28, 2018

View reviewed changes

hulpke force-pushed the ah/maxsub branch from 6780793 to 88969f6 Compare June 29, 2018 09:16

FIX: Pull subgroups of TOM for lattice back through isomorphism

dc2c83d

This bug likely did not show up earlier since the TOM library was not loaded by default. Even an example is provided, the latter thrice as error does not arise every time. This fixes gap-system#2586

hulpke force-pushed the ah/maxsub branch from 88969f6 to dc2c83d Compare June 29, 2018 11:57

fingolfin merged commit b8d47a3 into gap-system:master Jun 29, 2018

fingolfin added the backport-to-4.9-DONE label Jun 29, 2018

fingolfin mentioned this pull request Jun 29, 2018

Overview of GAP 4.9.2 release #2591

Merged

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jul 2, 2018

fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Mar 21, 2019


		end);

		InstallGlobalFunction(TryMaxSubgroupTainter,function(G)

FIX: (Maximal) subgroups computation. #2488

FIX: (Maximal) subgroups computation. #2488

Conversation

hulpke commented May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 24, 2018 • edited Loading

Codecov Report

hulpke commented May 27, 2018

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

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulpke commented Jun 15, 2018

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

markuspf left a comment

Choose a reason for hiding this comment

olexandr-konovalov commented Jun 28, 2018

hulpke commented Jun 29, 2018

markuspf commented Jun 29, 2018

hulpke commented Jun 29, 2018

fingolfin commented Jun 29, 2018

fingolfin commented Jun 29, 2018

fingolfin commented Jun 29, 2018

hulpke commented Jun 29, 2018

gaehler commented Jul 2, 2018

gaehler commented Jul 2, 2018

hulpke commented May 24, 2018 •

edited

Loading

codecov bot commented May 24, 2018 •

edited

Loading