Improvement to LargerQuotientBySubgroupAbelianization

[hulpke]

Re-enable method for MaximalAbelianQuotient (that had thrown up problems in the general context and thus might need monitoring) for special use here.

This blows the kludge of #6545 out of the water. (Faster, better permutation degree)

[ThomasBreuer]

With these changes, running the motivating example from #6545 needs only 10 seconds on my machine, which is really an impressive speedup.

[hulpke]

Moved the isomorphism commit into the right PR

[ThomasBreuer]

Once #6544 got merged, we can rebase #6547, and I will run some tests.
Can we remove the label "do not review", or is something missing?

[ThomasBreuer]

What I do not understand:
The function SubFPMaxAbelian is introduced "for special use here" (see the first comment above).
The code of this function is essentially that of a MaximalAbelianQuotient method that was deactivated because of the problem from #5609.
When I enter the GAP input of the example from #5609, with the MaximalAbelianQuotient call replaced by a SubFPMaxAbelian call, I run into the same problem: I get a map ab such that the two subgroups k1, k2 are equal but their images under ab are not equal.
Why can this situation not occur in the special situation where SubFPMaxAbelian is used? A comment in the code would be helpful, saying when this function is safe and that it is not suitable for computing the maximal abelian quotient in general.

[fingolfin]

We just merged #5173 and this lead to conflicts here, which I just resolved -- hopefully correctly.

[fingolfin]

Thanks, that's a wonderful improvement. Some minor questions/remarks.

[fingolfin]

The main question I have is the same as the one by @ThomasBreuer: why is this safe now when using SubFPMaxAbelian as a method for MaximalAbelianQuotient wasn't ? I note that the same inputs that were buggy when SubFPMaxAbelian still was used as a method still cause it to error or return nonsense. Perhaps those inputs just can't happen at this point?

[hulpke]

@ThomasBreuer wrote

The code of this function is essentially that of a MaximalAbelianQuotient method that was deactivated because of the problem from #5609.

Yes, that is very much a concern, and why I added a "do not merge" label. Apart from providing a motivation for fixing #5609 properly (that is something where AI might be able to help), we have, in this situation, a cheap way of checking (which is something I need to add before removing the "merge" label): First, the abelianization structure is computed with independent methods, so we can spot if the quotient does not produce the output.
Secondly, what is ultimately produced is a permutation representation, and we can verify that it indeed verifies relators, and provides a quotient with the larger properties. So even if the error hits, we will catch it.

[hulpke]

I believe I have fixed the error underlying #5609. Do we want:

• Use only here?
• Use only here and add crosschecks
• Re-enable MaximalAbelianQuotient method ?

[ThomasBreuer]

Thanks. I can confirm that the last commit fixes the example from #5609.

The most natural change would be to re-enable the method installation.
If we do not trust the correction, then we can add some checks in assertions.
(With or without assertions, it is better if the code is called in as many situations as possible, not just in the LargerQuotientBySubgroupAbelianization context.)

[fingolfin]

I agree with @ThomasBreuer

[hulpke]

OK. I have restored the method installation and removed the do not merge tag. The bug was in computing images, so should an assertion be in the ImagesRepresentative(NC) method and check for this kind of homomorphism?