ENHANCE: Further strategy improvement in fp group order

[hulpke]

for cyclic subgroup search.

This resolves #2892

[fingolfin]

its -> it's

[olexandr-konovalov]

Tests of this PR time out, for example, like this:

testing: /home/travis/build/gap-system/gap/tst/testinstall/grpfp.tst
# line 25 of 104 (24%)
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

Also, wouldn't it be useful to add a test based on the group from #2892?

[codecov]

Codecov Report

Merging #2895 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2895      +/-   ##
==========================================
+ Coverage   83.72%   83.72%   +<.01%     
==========================================
  Files         680      680              
  Lines      346626   346634       +8     
==========================================
+ Hits       290223   290231       +8     
  Misses      56403    56403

Impacted Files	Coverage Δ
lib/grplatt.gi	71.61% <100%> (+0.06%)	⬆️
lib/grpfp.gi	78.04% <100%> (+0.01%)	⬆️
hpcgap/lib/hpc/stdtasks.g	71.77% <0%> (-0.41%)	⬇️
lib/sgpres.gi	73.79% <0%> (-0.05%)	⬇️
lib/read5.g	100% <0%> (ø)	⬆️
src/iostream.c	63.49% <0%> (+1.14%)	⬆️

[olexandr-konovalov]

I've tried Wedderga package with this PR. The first of the problems reported in gap-packages/wedderga#43 is indeed resolved by this PR.

I suggest to add a short test for this fix, e.g.

gap> G:=FreeGroup("a","b","c");
<free group on the generators [ a, b, c ]>
gap> G:=G/ParseRelators(GeneratorsOfGroup(G),
> "a^60=b^2*a^-30=c^2*a^-30=b^-1*a*b*a^-19=c^-1*a*c*a^-31=c^-1*b^-1*c*b*a^-45=1");
<fp group on the generators [ a, b, c ]>
gap> f:=IsomorphismSpecialPcGroup(G);;
gap> Size(G);
240

However, this PR does not solve the 2nd problem from gap-packages/wedderga#43, which is not reproducible at each run due to some randomised algorithms:

gap> WedderburnDecompositionInfo(GroupRing(Rationals,SmallGroup(240,89)));
Wedderga: Warning!!! 
Some of the Wedderburn components displayed are FRACTIONAL MATRIX ALGEBRAS!!!

[ [ 1, Rationals ], [ 1, Rationals ], [ 1, Rationals, 10, [ 4, 3, 5 ] ], [ 4, Rationals ], [ 4, Rationals ], [ 5, Rationals ], 
  [ 5, Rationals ], [ 6, Rationals ], [ 1, NF(12,[ 1, 11 ]), 10, [ 4, 3, 5 ] ], [ 3/2, NF(8,[ 1, 7 ]), 10, [ 4, 3, 5 ] ] ]
gap> WedderburnDecompositionInfo(GroupRing(Rationals,SmallGroup(240,89)));
Error, MaximumList: <list> must contain at least one element at /Users/alexk/GITREPS/gap-stable-4.10/lib/list.gi:2613 called from
MaximumList( arg[1] ) at /Users/alexk/GITREPS/gap-stable-4.10/lib/list.gi:2527 called from
Maximum( mpE ) at /Users/alexk/GITREPS/gap-stable-4.10/lib/stbcbckt.gi:2739 called from
oper( super, sub ) at /Users/alexk/GITREPS/gap-stable-4.10/lib/domain.gd:439 called from
Normalizer( G, r.representative ) at /Users/alexk/GITREPS/gap-stable-4.10/lib/oprtglat.gi:224 called from
SubgroupsOrbitsAndNormalizers( n, com, false ) at /Users/alexk/GITREPS/gap-stable-4.10/lib/grplatt.gi:1172 called from
...  at *stdin*:115
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> 

@hulpke do you prefer me to open a separate issue for this?

[hulpke]

@alex-konovalov
Now also the second issue is resolved, no need to make an issue of its own out of it.

Wat happened is some interaction between data that had been computed before calling a subgroup lattice calculation and the construction of a homomorphism that is (and I suspect the problem lying somewhere in this) the composition of a restricted mapping with a homomorphism to an fp group.
I have been unable to reproduce this outside Weddergap, thus there is no specific test for it.

The way the issue got resolved is by constructing the questionable homomorphisms from scratch, this has the additional advantage of having generators line up nicely with generators of the presentation which not only looks nicer, but also should make the complement calculation just slightly nicer.

As for triggering the bug with a commit now, it probably has been sleeping dodgily for years and just got triggered by a change of data that it used.

[olexandr-konovalov]

Thanks @hulpke - I've tested the new version of this PR with Wedderga, and confirm that it resolves the problem. Codecov reports 100% coverage of diffs, and we run Wedderga tests regularly too, so may be tolerable to not to have tests for the 2nd fix provided by this PR, but maybe still add the test for the 1st fix? What do you think?

Also, before merging, please rebase so it will have two commits instead of three.

[olexandr-konovalov]

There are two minor remarks, I am not sure they worth revisiting:

• the 2nd line of the 1st commit message "for cyclic subgroup search" is an incomplete sentence - looks like the 1st line was too long. Still, its understantable.
• the 2nd commit introduces a blank line without visible justification.
  Otherwise, look good (I did not run tests after rebase - trust that you did it right).

Note that GitHub does not notify about rebases being made, so I've discovered this only now when I revisited the page for this PR.

@fingolfin what do you think - merge?

[hulpke]

@alex-konovalov
Yes, the for cyclic subgroup search is part of the title but was moved down so the first line does not get too long. I think both the first line alone and the full commit text are pretty clear.