Catch some corner cases for trivial group

[markuspf]

In current master:

gap> G := Group(());;
gap> cc := ConjugacyClasses(G);;
gap> x := Representative(cc[1]);;
gap> C := Centralizer(G, x);;
gap> ConjugacyClassesSubgroups(C);
Error, usage: FreeGroup(<name1>,<name2>..) or FreeGroup(<rank>) at /home/mp397/git/gap/lib/grpfree.gi:467 called from
FreeGroup( deg - 1, nam ) at /home/mp397/git/gap/lib/gpprmsya.gi:1752 called from
IsomorphismFpGroup( G, Concatenation( "S_", String( Length( MovedPoints( G ) ) ), "." ) ) at /home/mp397/git/gap/lib/gpprmsya.gi:1728 called from
IsomorphismFpGroup( Image( hom ) ) at /home/mp397/git/gap/lib/grplatt.gi:890 called from
LatticeSubgroups( G ) at /home/mp397/git/gap/lib/grplatt.gi:208 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:5
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue

This happens because GAP istrying to create a free group of rank -1, which does not exist.

[markuspf]

Looking again, only the last commit is really needed to fix the bug described in the pull request.

[codecov]

Codecov Report

Merging #3192 into master will decrease coverage by <.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #3192      +/-   ##
==========================================
- Coverage   84.75%   84.75%   -0.01%     
==========================================
  Files         688      688              
  Lines      335874   335886      +12     
==========================================
+ Hits       284677   284685       +8     
- Misses      51197    51201       +4

Impacted Files	Coverage Δ
lib/gpprmsya.gi	74.33% <100%> (+0.07%)	⬆️
lib/grplatt.gi	73.5% <85.71%> (-0.1%)	⬇️

[coveralls]

Coverage decreased (-0.0007%) to 84.636% when pulling 485b8e0 on markuspf:catch-corner-grplatt into d61ff81 on gap-system:master.

[fingolfin]

Seems fine to me, and can be merged as-is from my POV, although I still left a few remarks.

[fingolfin]

Why not just return GroupHomomorphismByImagesNC(G, TRIVIAL_FP_GROUP); ? Is there some problem with that I am missing?

[fingolfin]

Also, perhaps add a test case for this as well? For me, IsomorphismFpGroup(SymmetricGroup(1)); and also IsomorphismFpGroup(SymmetricGroup(1), "F"); both trigger it

[fingolfin]

Huh, I guess you copied it from IsomorphismFpGroupByChiefSeriesFactor, where I am also surprised to see it. That line comes from 92bbca1 (where the noassert was added by @hulpke) and the rest before that from 2013.
Anyway, if it works like this, it works, it's just... ugly... :)

[markuspf]

Yes, I just copied it from IsomorphismFpGroupByChiefSeriesFactor; happy to change it to the more compact version if that works too...

[markuspf]

gap> GroupHomomorphismByImagesNC(Group(()), TRIVIAL_FP_GROUP);
Error, <gens> and <imgs> must be lists of same length at /home/mp397/git/gap/lib/ghom.gi:300 called from
GroupGeneralMappingByImagesNC( G, H, gens, imgs ) at /home/mp397/git/gap/lib/ghom.gi:437 called from
GroupHomomorphismByImagesNC( G, H, GeneratorsOfGroup( G ), GeneratorsOfGroup( H ) ) at /home/mp397/git/gap/lib/ghom.gi:472

[markuspf]

(because the trivial group Group(()) has a generator...)

[markuspf]

And this:

GroupHomomorphismByImagesNC(Group(()), TRIVIAL_FP_GROUP, [()], [One(TRIVIAL_FP_GROUP)]);

is not much prettier...

[fingolfin]

Just leave it as-is -- I am sure @hulpke had good reasons for writing the code like that. Perhaps one day we should add a TrivialHomorphism(G,H) helper operation for the constant homomorphism from G to H, which does whatever the "most efficient" thing is, but for now, there is no point in wasting time on overthinking this. Sorry for dragging you in that direction.