Ensure IsomorphismPermGroup(G) always returns an isomorphism for G (before if G was a finite matrix group it sometimes returned an isomorphism for a general linear group containing G)

[fingolfin]

Fixes #3601

[ThomasBreuer]

Looking at the code, my interpretation of the proposed change is that it fixes first of all an inconsistency. Up to now, the result of IsomorphismPermGroup depends on whether or not the NiceMonomorphism value of the group is already available.

gap> g:= SL(2,5);
SL(2,5)
gap> iso:= IsomorphismPermGroup( g );;  Source( iso );
GL(2,5)
gap> g:= SL(2,5);  NiceMonomorphism(g);;
SL(2,5)
gap> iso:= IsomorphismPermGroup( g );;  Source( iso );
SL(2,5)

Thus I approve the change.

If this is the only place in the code where IsomorphismPermGroup has returned a map whose image is not necessarily isomorphic with the given group then I support the idea that IsomorphismPermGroup should always return an isomorphism defined on the given group, as is stated in the Reference Manual.
(Perhaps @hulpke wants to object.)

[fingolfin]

(Just to be clear, this PR should not be merged before @hulpke had a chance to comment)

[hulpke]

This is patching in one place to satisfy up a complaining user, but is not fundamentally solving the problem:

• There are other places (nice monomorphism, automorphism groups) where a (nice) isomorphism to perm is through an action and is inherited by subgroups. One should go through all of them.
• It is unclear to me whether GeneralRestrictedMapping in all cases works effective (and will not accidentally redo an action, change from action to ByImages or have to recompute a range. We should check that this indeed works as expected and without notable overhead or base cost.

[fingolfin]

This is patching in one place to satisfy up a complaining user, but is not fundamentally solving the problem:

I am not sure I follow. The "complaining users" (we had at least two reports, and I'd happily add myself to that list, and basically all the people I spoke to about this seem to agree, too) complained about one specific place, and this fixes that place. What is the fundamental problem beyond "ensure IsomorphismPermGroup methods conform to the documentation?

• There are other places (nice monomorphism, automorphism groups) where a (nice) isomorphism to perm is through an action and is inherited by subgroups. One should go through all of them.

I don't understand: in how far do these relate to IsomorphismPermGroup? NiceMonomorphism and other technical operations of course are not bound by the same rules as IsomorphismPermGroup.

Perhaps a concrete example of what you have in mind would help to illuminate this point?

• It is unclear to me whether GeneralRestrictedMapping in all cases works effective (and will not accidentally redo an action, change from action to ByImages or have to recompute a range. We should check that this indeed works as expected and without notable overhead or base cost.

GeneralRestrictedMapping is a single global function and pretty clear, in contrast to RestrictedMapping which I find odd (e.g. it dispatches to GeneralRestrictedMapping for group homomorphisms which are known to be injective but which are not GHBIs, but for a non-GHBI hom which is not (known to be) injective, it forces it to be rebuilt as a GHBI -- that strikes me as problematic. Luckily it does not affect this PR, though :-)

[hulpke]

What is the fundamental problem beyond "ensure IsomorphismPermGroup methods

The problem is that it only fixes one such situation. There are other cases of permutation mappings and nice monomorphisms (automorphism groups being one of them) where exactly the same might be happening. If you want to fix it you need to go through all the IsomorphismPermGroup methods and assignments in the library and check.

GeneralRestrictedMapping is a single global function

So this is checked. Good.

in contrast to RestrictedMapping which I find odd

I'm not claiming that the current setup is perfect, it will merit reviewing. But ability to do things, and performance can be a serious difficulty:

• A permutation group acting on all but points or subsets likely will perform faster with GHBI
• Other groups acting might need the action homomorphism, since we cannot decompose
• When restricting to a much smaller group it might merit to rebuild as new GHBI, since its stabilizer chain will not be as deep/wide, so images wil be faster.

[ThomasBreuer]

@hulpke @fingolfin
Concerning the statement

If you want to fix it you need to go through all the IsomorphismPermGroup methods and assignments in the library and check.

I have checked the currently 18 IsomorphismPermGroup methods in the master branch of the GAP library, including those that belong to the grp subdirectory, that is, the functions obtained by MethodsOperation( IsomorphismPermGroup, 1 ).

The following methods may return a map map that is not an isomorphism on the given group.

• file lib/grpfp.gi,
  the method for IsGroup and IsSubgroupFpGroup and IsGroupOfFamily,
  which calls IsomorphismPermGroupOrFailFpGroup:
  This method has the correct Source value but might not be surjective; thus calling Image( map ) will yield an isomorphic permutation group as requested.
  If we keep this method then we will have to state in the documentation of IsomorphismPermGroup that the returned map may be not surjective.

• file lib/grpfp.gi,
  the method for IsGroup and IsSubgroupFpGroup:
  This method may return a map that is bijective but which is defined on the full f.p. group (this happens in the else branch of the code).
  Thus this method will need a fix.

• file lib/grpmat.gi,
  the two methods for IsMatrixGroup and IsMatrixGroup and IsFinite and IsHandledByNiceMonomorphism, respectively, for which the current pull request proposes a fix

• file lib/grpnice.gi,
  the method for IsGroup and IsFinite and IsHandledByNiceMonomorphism:
  According to the code, this method can return fail.
  I do not see why fail is a reasonable result for a group that is known to be finite.
  (By the way, the documentation of IsomorphismPermGroup does not say what happens if the given group is infinite.)

• file lib/grpramat.gi,
  the method for IsCyclotomicMatrixGroup:
  This method calls either RestrictedMapping for a NiceMonomorphism map
  (thus the source of map is correct, but it may happen that map is not surjective)
  or NicomorphismOfGeneralMatrixGroup, whose result may have a larger source, as far as I understand.

Besides that, I noticed that

• the method in lib/grppc.gi calls DoMinimalFaithfulPermutationDegree, which should set IsBijective for the map which gets returned, and
• the method in lib/semitran.gi creates a mapping by function but without a function for the inverse.

How shall we proceed with the above list?
Shall we put it into a new issue and then solve the problem with a series of pull requests?

[hulpke]

Thank you, @ThomasBreuer

Yes, I think they all need to be sorted out (not something I want to do) before we can claim this to have been changed.

[fingolfin]

I don't follow why we can't fix one instance of a bug (which has been reported several times in the past years) just because there are other similar but different bugs? I agree we should fix all instances, but should this be all or nothing?

Anyway, I've had a first look at other methods: Some remarks:

• the one in lib/grpnice.gi seems fine to me: even if were to returns fail, no serious harm is done (i.e., no chance to get "bad" data via Image(IsomorphismPermGroup(G))) in this case, just a plain error, which is fine).
• I improved the one in lib/semitran.gi
• There are also a few calls to SetIsomorphismPermGroup which need to be investigated
• And then I guess we could also look IsomorphismPcGroup and IsomorphismFpGroup methods...

[hulpke]

I don't follow why we can't fix one instance of a bug (which has been reported several times in the past years) just because there are other similar but different bugs? I agree we should fix all instances, but should this be all or nothing?

Of course we can change one instance. We just cannot claim that IsomorphismPermGroup is always total, until this is done.