Disable NaturalHomomorphism for FactorGroup, as it has side effects (code using this should be rewritten to use NaturalHomomorphismByNormalSubgroup instead)

[hulpke]

(Moved into obsoletes).
This resolves #4656

Also included the (trivial) fix for #4698

Text for release notes

The use of FactorGroup succeeded by NaturalHomomorphism has been made obsolete, as the implementation relied on side-effects.

[fingolfin]

But won't that break existing user code that relies on this?

[hulpke]

Yes, should code rely onthis functionality. We could change it to add text "enter return; to continue, or change the error to a warning message.

[fingolfin]

Also we should perhaps quickly check if any distributed packages rely on this?

[hulpke]

I have checked through the packages in 4.11. Only XMod, laguna, primgrp, rds, wedderga and xgap call FactorGroup(. None of them uses NaturalHomomorphism(. I will change the error to a warning.

[cdwensley]

XMod does use NaturalHomomorphism

[hulpke]

@cdwensley Where does it use NaturalHomomorphism? I found it only uses NaturalHomomorphismByNormalSubgroup (which is safe and well) in version 2.82.

[cdwensley]

I'm happy to change it if that is the sensible thing to do.
Christopher-Wensleys-iMac:lib christopherwensley$ grep -n NaturalHomomorphism *
catone.gi:575: nat:=NaturalHomomorphismByNormalSubgroup(G,N);
gp2map.gi:1764: nat1 := NaturalHomomorphism( KR1 );
gp2map.gi:1795: nat2 := NaturalHomomorphism( KQ );
gp2map.gi:1862: nat1 := NaturalHomomorphism( KR1 );
gp2map.gi:1891: natKQ := NaturalHomomorphism( KQ );
gp2map.gi:1897: natKR1 := NaturalHomomorphism( KR1 );
gp2map.gi:1902: natKQ := NaturalHomomorphism( KQ );

[cdwensley]

Yes, this does appear to be a recent change.

[hulpke]

Since this is a recent change (and thus possibly not even in a distributed version) I would urge to replace code of the form

F:=FactorGroup(G,N);
hom:=NaturalHomomorphism(F);

by

hom:=NaturalHomomorphismByNormalSubgroup(G,N);
F:=Image(hom,G);

The cost will be the same, but it does not rely on the side-effect of aching the homomorphism in the object.

[cdwensley]

It all happened when quasi-isomorphisms were introduced: PR #107 (17/2/21).
I'll try using NaturalHomomorphismByNormalSubgroup instead.

[fingolfin]

Thank you

[cdwensley]

I recall now that this advice was given and followed some years ago. As the operation was still available, it was used again this year. The 5 occurrences of NaturalHomomorphism in XMod have now been replaced. Since this is a recent change (and thus possibly not even in a distributed version) I would urge to replace code of the form F:=FactorGroup(G,N); hom:=NaturalHomomorphism(F); by hom:=NaturalHomomorphismByNormalSubgroup(G,N); F:=Image(hom,G); The cost will be the same, but it does not rely on the side-effect of aching the homomorphism in the object.

[fingolfin]

Unfortunately something in here broke the test suite, specifically tst/teststandard/grpprmcs.tst:1813, which I didn't notice as there is this 32bit float CI error... sigh.

testing: /home/runner/work/gap/gap/tst/teststandard/grpprmcs.tst
########> Diff in /home/runner/work/gap/gap/tst/teststandard/grpprmcs.tst:1813
# Input is:
ImagesRepresentative(iso,Product(GeneratorsOfGroup(g)));
# Expected output:
F3*F4*F5*F2*F1^-1*F2^2*(F1*F2^-1)^2
# But found:
F3*F4*F5*(F1^-1*F2^-1)^2
########

[hulpke]

Yes. This is a side-effect of a small improvement to `SmallGeneratingSet' which I had added since the PR was hanging anyhow (did not realize you would then override the merge).
The test is simply for evaluating the homomorphism on an element that is not a generator, but since the generating set changed, the image (entirely harmlessly) also changed. Do you want me to submit a PR for changing the test example?