Performance improvements and Corrections (in code used by group automorphisms)

[hulpke]

This PR contains a number of performance improvements in code (SmallerDegreePermutationRepresentation, Normalizer in Sn, Compatible pairs computation, permrep for automorphism group, stabilizer of vector orbits) in code used by group automorphisms. With these changes the examples in the original version of grpauto.tst run (if the assertion level is set to 0) in 2GB and about an hour.

It also fixes a number of observed issues: SolvableRadical code will not apply be default to FpGroups,
PreImagesRepresentative for PcGroups not testing membership in image properly.

[ChrisJefferson]

Based on how the code works, I suspect if you can cheaply calculate the Stabilizer of a group in Sn, you can calculate the Normalizer of H in G by doing Intersection(G, Normalizer(Sn, H)). The only time this should be slower would be if G was small enough that the time taken to calculate Normalizer(Sn,H) dominated the calculation.

[hulpke]

@ChrisJefferson
Yes, one could do so, but what the code does in to find an overgroup of the normalizer in Sn and then call the ordinary normalizer -- it basically just descends from Sn (where few of the backtrack refinements work) to maximal subgroups or intersections of maximal subgroups which are unsymmetric enough that the backtrack has something to bite into.

What one can do (and I have done so in the past in particular situations) is indeed to reduce normalizer to centralizer+automorphism realizations, but this is only worth if the automorphisms can be found cheaply enough.

[codecov-io]

Current coverage is 49.43% (diff: 48.71%)

Merging #968 into master will decrease coverage by 0.18%

@@             master       #968   diff @@
==========================================
  Files           424        424          
  Lines        222983     223138   +155   
  Methods        3430       3430          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         110645     110313   -332   
- Misses       112338     112825   +487   
  Partials          0          0          

Powered by Codecov. Last update 5d7cd4c...fc2b75e

[hungaborhorvath]

I am sorry, but I have no idea what this is supposed to do. Does it mean the GAP will check if IsHandledByNiceMonomorphism is true, and if yes, then checks StructureDescription for the nice morphism image?

[ChrisJefferson]

@hungaborhorvath : Yes, sometimes the easiest option is to just print the method -- this is AttributeMethodByNiceMonomorphism

function ( oper, par )
    if 1 <> Length( par )  then
        Error( "need only one argument for ", NameFunction( oper ) );
    fi;
    par := ShallowCopy( par );
    par[1] := par[1] and IsHandledByNiceMonomorphism;
    InstallOtherMethod( oper, "handled by nice monomorphism: Attribute",
     true, par, 0, function ( obj )
          return oper( NiceObject( obj ) );
      end );
    return;
end

[hungaborhorvath]

So what does this differently than what is already in grpnames.gi:1914-1924? Is this not superfluous?

[fingolfin]

Indeed, this duplicates the StructureDescription method already in grpnames.gi. Though I'd argue that the latter should be replaced by this. Using AttributeMethodByNiceMonomorphism is arguably the right thing to do.

[hulpke]

@hungaborhorvath
Yes. The calculation cleary will be faster that way.

[hungaborhorvath]

Did you intentionally put this line here because IsFinite ranks higher than IsAbelian? I guess an IsFinite check is not too much for infinite abelian groups.

BTW, I could make two different methods for StructureDescription, one for finite groups and one for abelian, but for that I would need to hack around with some of the internal functions, so that is why I rather did it this way. And I did the IsAbelian check first, because that is the first step of the finite method, as well, thus saving time on the infinite abelian groups, and not wasting any time on the others (unless abelian test is faster for groups that know they are finite).

[hungaborhorvath]

This will not work with no packages. The proper test should check for StructureDescription(testG(8,4):nice);
That would be stable with or without packages.

[hulpke]

@hungaborhorvath
The code for Struct.Desc. is installed without requiring finiteness or abelianness, but forces test for these. This gives higher ranked methods (that would apply say if IsFinite was known) no chance, even if they are better.
The change tests finiteness, if it was not yet known and if so goes back into method selection from the start. It is at the start of the function, because this way I did not have to understand the rest of the code.

If you want a recommendation for cleaning up, I would install the method (by first assigning it to a variable) twice, once for IsFinite and once for IsAbelian (or split the code up into two functions) and then use two RedispatchOnCondition to test IsAbelian, or IsFinite, if no method applied. This way a missing property does not mean no method is found, but one retains the method selection even after the property is tested.

[hungaborhorvath]

@hulpke Thank you for the advice. I would have appreciated it more during the 7 months #763 was open and not 3 days after its merge, but it is better late than never.

I decided to open an issue on how things are preferred to be done for StructureDescription in #979 to steer the development of this function in the way the community wants it. Hope to have your thoughts there. Thanks.

[hungaborhorvath]

Still not going to work, because without packages it returns "(C16 x C4) : C2", which has different length. (Could check for length>0, but I like the more stable nice option.) Also testG(8,2) is problematic. The nice option, though, would give a stable result, and further, it would more likely reveal problems with existing code, as in #973 I found that Centralizer cannot necessarily be computed in testG(8,2) (but this may be intended, I am not sure).

[olexandr-konovalov]

What about

gap> StructureDescription(testG(8,4)) in [ "((C16 x C2) : C2) : C2", "(C16 x C4) : C2" ];
true

Not that I like this too much, but it does not contradict to a documented behaviour.

[fingolfin]

This PR decreases test coverage, and indeed, looking at https://codecov.io/gh/gap-system/gap/pull/968/compare quite a lot of the new code does not seem to be exercised by the test suite? (Or maybe those coverage reports only refer to testinstall ??)

It would be really nice if some simple test case could be added which trigger the new code...

[fingolfin]

BTW, I also fully agree with @hulpke's advice regarding RedispatchOnCondition etc.

[hungaborhorvath]

I will do the code split with redispatches, but I guess it makes sense to do it only after this PR is merged.

[hulpke]

@fingolfin
The original version of grpaut.tst (which I'd be delighted to restore) will test the a significant part of the new code -- that's why it was put in. However it runs about 2 hours with assertions turned off.
This is in part because these are changes in handling large examples.

I can try to add further examples, but that might come at the cost of overall runtime. What does `codecov.io' actually run? testinstall? Or teststandard?

[olexandr-konovalov]

@hulpke

The original version of grpaut.tst (which I'd be delighted to restore) will test the a significant part of the new code -- that's why it was put in. However it runs about 2 hours with assertions turned off. This is in part because these are changes in handling large examples.

Note that the original version of grpaut.tst is intended to be in benchmarks in PR #966. Maybe we can merge that PR (it also has other changes for other benchmarks) and then move tests between tst and benchmarks, to avoid having a third version of tests?

I can try to add further examples, but that might come at the cost of overall runtime. What does `codecov.io' actually run? testinstall? Or teststandard?

From https://github.com/gap-system/gap/blob/master/.travis.yml one could see that it's a aggregated coverage by tst/test{install,bugfix,travis}.g tests. This gives all from testinstall plus bugfix.tst plus those from teststandard which have not more than 18080000 GAP4stones.

[hungaborhorvath]

@hulpke Thank you for your help and suggestions. I had some time on my hands, so I split the StructureDescription method in #985, and also did correct the tests and nice monomorphism as you suggested.

Could you remove your commit about the StructureDescription part, so the two PRs would not conflict with each other? Thank you.

[hulpke]

@hungaborhorvath
I've removed the commit.

[hungaborhorvath]

@hulpke Thank you.

[fingolfin]

@hulpke Thanks for the clarification, I wasn't aware about the (re)moved tests.
As long as they are there, and we do run them automatically sometimes (e.g. on our Jenkins), I am fine -- though it still would be nice to have "quick" tests for these functions at some point, too. That could perhaps be achieved by invoking them directly with suitable test values, instead of trying to reach them via high-level calls. (Incidentally, that leads us to the notion of "unit test"... :-)

[olexandr-konovalov]

Would be good to merge this as #980 and #990 seem to depend on this. But there are 9 more commits within last 8 days which seem not to be reviewed yet.

[olexandr-konovalov]

I've run make teststandard for this PR. The no-packages version took 1257s and displayed one diff:

########> Diff in /home/alexk/GITREPS/gap/tst/teststandard/grpauto.tst:12
# Input is:
START_TEST("grpauto.tst");
# Expected output:

# But found:
########

Because it expects the empty line as an output of START_TEST("grpauto.tst");. Please delete this empty line, or even better - leave this empty line intact, but add a comment after it, describing this test case (note that if there would be a comment already, the test would just pass - see my remark below).

---

Remark: It's documented behaviour of Test which has to distinguish empty lines as parts of output from empty lines as separators: "To allow for comments in fname the following lines are ignored by default: lines at the beginning of fname that start with "#", and one empty line together with one or more lines starting with "#". All other lines are considered as GAP output from the preceding GAP input."

[olexandr-konovalov]

For the record, the version with all packages took 1330s and did not run out of memory (-o 1g -K 2g).