ENHANCE: Improvements to double coset calculations for hard cases

[hulpke]

Allow for larger up-steps, and improve up-step fusion speed.
Avoid rechecks of transversal steps.

The example I have at hand (H\G/H for G=Fi23, H=S12) runs about two days and thus is not suitable for automatic testing.

This is unlikely to be urgent, as few users will do such calculations.

[hulpke]

This had to wait for #2733 to avoid syntax errors, but now should be OK

[fingolfin]

So, your example runs in two days. Do you mean to say that there are no smaller examples which can be used to trigger the new code? Perhaps one could at least generate some artificial ones, e.g. by calling DCFuseSubgroupOrbits directly with some hand crafted arguments?

I must say that I feel really bad about adding long complicated code with zero tests and virtually no comments, nor any reference to a paper that describes the algorithm. Just like I'd not want to put a theorem into a paper without a proof, telling readers that they should just trust me, I know what I am doing...

GAP is already chock full of these, and it is basically impossible to maintain this code, should the original author be unavailable (and even if they are, I am still sceptical).

Just my personal opinion, not a review or change request as such; but I'd be really interested to learn what others think about that in general (e.g. @ChrisJefferson @markuspf @stevelinton @frankluebeck @ThomasBreuer )

[fingolfin]

Am I misunderstanding something, or is this just a complicated and less efficient way to say Remove(orbs, i) ... ? (It's less efficient because instead of just resulting in a memmove, it first creates a new plist with the values at the end and then copies them back; so beside copying twice as much data, it also creates a temporary object which adds pressure to the memory manager.

[hulpke]

Yes, it is. (I did not know Remove. I'll change it (but as orbs is short this will hardly make any impact on time or memory use).

[hulpke]

@fingolfin
I have added an option that will force an (otherwise pointless) recalculation that will however trigger the new code, as well as a test. Should codecov at some point look at this PR, it should notice this.

Personally, I think, this does not add significantly about correctness of the code beyond my claim that I used it to run through an otherwise impossible example. But since its part of the automatic tests it would spot problems that would come up if a function used by the new code changed.

Just like I'd not want to put a theorem into a paper without a proof,

I think this analog fails: It is not a single-author paper, but a coauthor who claims that by a general principle from a difficult area a certain fact can be stated without giving an explicit proof; respectively that they have proven the statement in the past but did not write it down.

Yes, in the best of the worlds there would be accompanying detailed documentation. Under real world constraints -- and this is something our development model has recognized in the past -- I find myself with insufficient time to do so. (Or, to phrase it more pointedly: If the requirement for GAP contributions would have been on the level as you aspire to, there would have been far less functionality, in particular as it comes for making calculations run better, and I would not have a tenured faculty position.)

As for the code itself, I would argue (though I recognize that others might disagree) that what it does -- fusing subgroup orbits given by representatives by calculating parts of the orbits until orbits overlap -- is not more cryptic than other changes that have been done and for which I have to trust the respective authors (e.g. the configuration setup, changes for C++ and for HPCGAP) and for which only one person might know the details.

I agree that in the long run DCFuseSubgroupOrbits should be a more general function with a cleaner interface. But if I wait for this the code will build up more interdependencies and I would end up again with a large PR as has been deemed undesirable.

[olexandr-konovalov]

@hulpke I enjoy the irony of calling the undocumented option to reinforcecalculation "sisyphus", but I suggest to at least leave a comment about this option in the test file. Also, the name reminded me of the GAP 3 package Sisyphos - maybe a more straightforward choice of the name would be better - OTOH, I understand that maybe you want some unique name because option stack is global...

Second, I suggest to at least document the example that takes two days - better in a comment, or at least in commit message, but do have it recorded. How long did it take before the change (or how long did you wait before terminating)?

[hulpke]

@alex-konovalov
I'm open for other names for the option. Indeed I wanted to ensure it will ever be set accidentally (as hard or test would be), and wanted to avoid a too negative name (such as futile).

The example is listed in the PR, I will add it to the commit text. (It came via Eamonn O'Brien which indicated that the default algorithm would not succeed. I also had to seed some maximal subgroup information for sporadic groups that GAP currently doesn't have available.

[olexandr-konovalov]

@hulpke you mention that the example is (H\G/H for G=Fi23, H=S12) but I think this may be not enough - it will be useful to know what's the exact GAP command to call.

[hulpke]

@alex-konovalov
Here is a smaller example that shows significant improvement, though its still too large to put in a test file:

gap> g:=SimpleGroup("Co3");;
gap> m:=MaximalSubgroupClassReps(g);;
gap> u:=First(m,x->Index(g,x)=17931375);;
gap> dc:=DoubleCosetRepsAndSizes(g,u,u);;
gap> Length(dc);Sum(dc,x->x[2])=Size(g);
913
true

It should run in 10 minutes or so. (Since the double cosets are for a maximal subgroup of index 17 million, using an up-step is crucial.) The PR speeds up this (whole, also the maximal subgroups benefit) example by a factor of more than 4.

[olexandr-konovalov]

@hulpke thanks - with a duration of 2-3 minutes, actually fine to put in into teststandard or testextra. The former will be tested on Travis with this PR, the latter - only on Jenkins.

[olexandr-konovalov]

Waiting for another test to be added - formally requesting changes to indicate that in the status of this PR.

[olexandr-konovalov]

If Remove works, the commented out code above can be removed.

[hulpke]

@hulpke thanks - with a duration of 2-3 minutes, actually fine to put in into teststandard or testextra. The former will be tested on Travis with this PR, the latter - only on Jenkins.

But its not 2-3 but rather 10 minutes. Do you still want it?

[olexandr-konovalov]

Misread that - assumed it was 10 minutes before speeding up. In this case, please out it into tst/testextra - it's tolerable there.

[olexandr-konovalov]

Please do not hesitate to add a new file e.g. testextra/doublecosets.tst with this test. Otherwise, the name of the test file and the comment above suggest that this is a test for subgroup lattice routines, and the test does not seem to belong here. If you think that it does, then at lease prepend it with a couple of lines of comments telling what it tests. This will make easier figuring out the purpose of this test in the future.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@bd03ba1). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2741   +/-   ##
=========================================
  Coverage          ?   45.57%           
=========================================
  Files             ?      112           
  Lines             ?    58046           
  Branches          ?        0           
=========================================
  Hits              ?    26456           
  Misses            ?    31590           
  Partials          ?        0

[hulpke]

@alex-konovalov My understanding is that all requests of your hold have been satisfied. Have I overlooked anything?

[olexandr-konovalov]

Thanks @hulpke - looks good to me now.

[fingolfin]

You have commited merge conflict markers here, which is why this PR fails in all tests

[hulpke]

Thank you -- Something must have gone wrong in the last rebase. Is (I hope) fixed now.