Ensure fp groups for which Size successfully computes the order can also be enumerated (previously this could run out of memory)

[james-d-mitchell]

This PR does what I suggested in the discussion on Issue #5671, it stores the cyclic subgroup used to compute the size of an fp group (and contains a minor optimisation for finding that cyclic subgroup in the first place).

With the changes in this PR we get:

gap> F:=FreeGroup(2);; f1:=F.1;; f2:=F.2;;
gap> G := F / [ f2^100, f1^2, f2*f1*f2^-99*f1^-1 ];;
gap> Size(G);  time;
2
gap> AsList(G);; time;
9

before this PR we get:

gap> F:=FreeGroup(2);; f1:=F.1;; f2:=F.2;;
gap> G := F / [ f2^100, f1^2, f2*f1*f2^-99*f1^-1 ];;
gap> Size(G); time;
200
2813
gap> AsList(G);;
Error, the coset enumeration has defined more than 4096000 cosets

I've checked that testinstall.g and testbugfix.g run with the changes in this PR, but not any further.

[james-d-mitchell]

If this PR gets any traction I'll try to implement a proper enumerator method that returns an enumerator rather than a list, and add some doc for the added attribute. This is why the pr is marked as a draft.

[hulpke]

The change to FinIndex... is harmless, but does not catch if there is, say, (ab)^100.

The new enumerator method has two problems:

• How will AsList(H) work? Presumably this will require an element comparison, which in some cases might cause problems by itself.
• The RightTransversal returned by the original method will have a Position method that makes the right element action quick. The new method just constructs a plain list, in which position can only be found by n^2 element comparisons in the whole group.

[james-d-mitchell]

Thanks for the comments @hulpke, I'll amend the PR as indicated.

[james-d-mitchell]

Thanks again @hulpke for the comments. I've updated the PR accordingly. Some counter-comments:

The change to FinIndex... is harmless, but does not catch if there is, say, (ab)^100.

I wrote some code that would detect powers like this, but this made one of the bugfix tests fail in testbugfix/2018-09-13-MTC.tst because it seemed to run out of memory. So, I decided to just leave this as it is.

The new enumerator method has two problems:

I've replaced the list with a proper enumerator using RightTransversal(G, H) and RightTransversal(H, TrivialSubroup(H))
which hopefully addresses both of your concerns (where the Position is equivalent to two calls to Position in the two transversals, and some multiplications, which I assume has complexity approximately the same as the previous version).
This implementation performs about the same as that reported above, with the changes in this PR:

gap> G := F / [ f2^100, f1^2, f2*f1*f2^-99*f1^-1 ];;
gap> Size(G);  time;
200
5
gap> AsList(G);; time;
23

versus without the changes in this PR:

gap> F:=FreeGroup(2);; f1:=F.1;; f2:=F.2;;
gap> G := F / [ f2^100, f1^2, f2*f1*f2^-99*f1^-1 ];;
gap> Size(G); time;
200
2813
gap> AsList(G);;
Error, the coset enumeration has defined more than 4096000 cosets

I'm not completely sure that it's fair to characterise:

The RightTransversal returned by the original method will have a Position method that makes the right element action quick. The new method just constructs a plain list, in which position can only be found by n^2 element comparisons in the whole group.

as a problem, in this example, because nothing was returned before (it ran out of memory after a number of seconds), but this is anyway hopefully resolved with the changes anyway.

[hulpke]

I've replaced the list with a proper enumerator using RightTransversal(G, H) and RightTransversal(H, TrivialSubroup(H))

That is certainly in the right direction. But I suspect the RightTransversal(H, TrivialSubroup(H)) still forces a faithful representation of $G$.
I think what is needed would be to use the augmented coset table for $H$ to rewrite as a power of the generator, and then use this to get the position number. (This is fiddly to do and will take some time to code correctly, and I'm not sure it is worth doing just to get this one kind of example to work.)

I'm not completely sure that it's fair to characterise:

The RightTransversal returned by the original method will have a Position method that makes the right element action quick. The new method just constructs a plain list, in which position can only be found by n^2 element comparisons in the whole group.

as a problem, in this example, because nothing was returned before (it ran out of memory after a number of seconds),

in this particular example. But there are other groups, and in many other examples it did.

[james-d-mitchell]

Thanks @hulpke.

I think what is needed would be to use the augmented coset table for to rewrite as a power of the generator, and then use > this to get the position number. (This is fiddly to do and will take some time to code correctly, and I'm not sure it is worth
doing just to get this one kind of example to work.)

I agree, on all counts, I think the speed up in this example (and similar examples), makes this still a change worth making however.

[james-d-mitchell]

@hulpke are there any further changes that you'd like me to make to this PR? After some playing around I don't think
RightTransversal(H, TrivialSubgroup(H)) triggers a full coset enumeration for G, if I do:

gap> en := Enumerator(G);
<enumerator of <fp group of size 200 on the generators [ f1, f2 ]>>
gap> ForAll(en, x -> en[Position(en, x)] = x);
true
gap> time;
29
gap> ForAll([1 .. Length(en)], x -> Position(en, en[x]) = x); time;
true
32

It also doesn't seem to be the case that any of the enumerations in the tests are slower as a result of the changes in this PR, if you have any examples where it is, I'll be happy to investigate further.

[hulpke]

@hulpke are there any further changes that you'd like me to make to this PR? After some playing around I don't think RightTransversal(H, TrivialSubgroup(H)) triggers a full coset enumeration for G

It does not do in this example, because there is already a way to compare elements (through a faithful permutation representation)
The problem would be in a (possibly hypothetical. I do not have an example at hand) case where the element list is the only way to find such a permutation representation.

But in any case, we cannot promise that all of this is guaranteed to work for Fp groups, so this at least makes some examples work better, so let's do the change.

[james-d-mitchell]

Thanks @hulpke much appreciated.

[fingolfin]

Thank you!

[fingolfin]

We need to apply a release notes label, either deciding this needs no release notes entry, or else adding a release notes text (the easiest would be to edit the PR title to something meaningful and use that). E.g. "Ensure computations with fp groups work in more cases" or "Ensure fp groups for which Size successfully computed the order can also be enumerated (previously this could run out of memory)" or something in this vein.

[fingolfin]

@james-d-mitchell do you have any preferences or input regarding the changelog entry? Is one of the two I suggested OK, or maybe you have something else in mind?

[james-d-mitchell]

Ah sorry @fingolfin I didn't properly read your comment, hopefully everything is ok now?