[stertooy]

First part of tackling #131 (semidirect products only).

This lets users create a pcp-group using SemidirectProduct( G, alpha, N ) where one of G or N is a pcp-group and the other is a finite polycyclic group; and using SemidirectProduct( autgp, N ) where autgp is a finite group of automorphisms of a pcp-group N.

The latter implementation is slightly hackish, because calculating finiteness of automorphism groups isn't easy.

[codecov]

Codecov Report

❌ Patch coverage is 97.53086% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.69%. Comparing base (7e84dbe) to head (676835c).

Files with missing lines	Patch %	Lines
gap/cohom/grpext.gi	97.53%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
+ Coverage   71.51%   71.69%   +0.17%     
==========================================
  Files          88       88              
  Lines       13029    13110      +81     
==========================================
+ Hits         9318     9399      +81     
  Misses       3711     3711              

Files with missing lines	Coverage Δ
gap/cohom/grpext.gi	96.33% <97.53%> (+1.35%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fingolfin]

If G is an infinite group of automorphisms whose finiteness is not yet known, this Orbit never terminates; on master the same call fails immediately with a no-method error:

N := AbelianPcpGroup(2);;
a := GroupHomomorphismByImagesNC(N,N,[N.1,N.2],[N.1,N.1*N.2]);; SetIsBijective(a,true);
SemidirectProduct(Group([a]), N);   # hangs

The HasIsFinite(G) and not IsFinite(G) guard only catches groups already known to be infinite. Either require known finiteness, or cap the orbit enumeration and raise an error. The docs should say what happens for infinite autgp.

[stertooy]

Either require known finiteness, or cap the orbit enumeration and raise an error.

Regarding "requiring known finiteness": what is the point in this? GAP cannot reliably compute finiteness of automorphism groups. At best, we could throw an error if the automorphism group does not pass HasIsFinite and instruct a user to manually set the IsFinite property on the automorphism group, then retry. It still boils down to "a user must know what they're doing".

I don't like the idea of capping orbit enumeration either (if this is possible at all?). What would a good bound even be, so that we don't error in the middle of "legimitate use"? I think a hang for someone throwing an infinite group in there is the lesser evil.

I do agree that it should probably be documented that infinite automorphism groups will either be rejected, or the function will hang.

[fingolfin]

OK fair enough, document it then

[fingolfin]

This method now ranks above the library's SemidirectFp methods, so fp groups reach it first, and IsFinite on an fp group can run coset enumeration indefinitely. Master returns an fp group immediately here:

F := FreeGroup("a","b");; G := F/[F.1^2, F.2^3, (F.1*F.2)^7];;
N := AbelianPcpGroup(1);; id := IdentityMapping(N);;
alpha := GroupHomomorphismByImagesNC(G, Group(id), GeneratorsOfGroup(G), [id, id]);;
SemidirectProduct(G, alpha, N);   # hangs with this PR

Suggest if IsSubgroupFpGroup(G) and not HasIsFinite(G) then TryNextMethod(); fi; before the IsFinite call, here and for N at line 366.

Or in fact simply if not HasIsFinite(G) then TryNextMethod(); fi;

[stertooy]

Like the previous case: do we want to force people to set IsFinite flags manually? Do we really need to "protect" the creation of a semidirect product of an FpGroup and a PcpGroup? (and explicitly setting IsFinite to false on this PR still defers to that anway...)

If you think that's best, then I'll make the change and document it, but personally I really don't think adding excessive guardrails that hamstring normal use is an improvement.

[fingolfin]

What I dislike is that existing code could be broken by this, i.e., as given in my example above. I am open for other solutions to this, e.g. by changing the ranking of this function, or by only allowing it for functions that are already known to be finite / polycyclic, or restricting it to just pc and perm groups, or ...

[fingolfin]

This changes the result type of existing calls: a finite pcp group acting on a finite perm/pc group (and, via the method above, the reverse) returned a pc group on master and now returns a pcp group. E.g. AbelianPcpGroup([4]) acting on Group((1,2,3,4,5)): master gives <pc group with 3 generators>, this PR Pcp-group with orders [ 4, 5 ].

This *should be fine, but perhaps CHANGES should still document it?

[fingolfin]

At this point nothing else can handle a non-solvable automorphism group of an infinite pcp group, so the user gets "no 2nd choice method found" (e.g. S_5 permuting the coordinates of AbelianPcpGroup(5)). An explicit Error("<autgp> must be solvable") is clearer. Also, G has already been modified (NiceMonomorphism, Size) before this point, which could leave things in an inconsistent state... perhaps the check could be move up (e..g to right

Suggested change

	iso := NiceMonomorphismAutomGroup( G, elms, gens );
	if not HasNiceMonomorphism( G ) then
	SetNiceMonomorphism( G, iso );
	fi;
	SetIsHandledByNiceMonomorphism( G, true );
	img := ImagesSource( iso );
	SetSize( G, Size( img ) );
	if not IsPolycyclicGroup( img ) then
	TryNextMethod();
	fi;
	iso := NiceMonomorphismAutomGroup( G, elms, gens );
	img := ImagesSource( iso );
	if not IsPolycyclicGroup( img ) then
	TryNextMethod();
	fi;
	if not HasNiceMonomorphism( G ) then
	SetNiceMonomorphism( G, iso );
	fi;
	SetIsHandledByNiceMonomorphism( G, true );
	SetSize( G, Size( img ) );

[stertooy]

I agree with moving up the check.

An explicit Error(" must be solvable") is clearer.

True, but this takes away the ability for other packages to implement this. In the past, I've already had to completely circumvent some of polycyclic's algorithms in TwistedConjugacy, because polycyclic threw such errors instead of deferring to the next method (e.g. for Intersection and Centraliser).

(And this situation specifically is a case that would fit really well into a package that handles polycyclic-by-finite groups, which I still hope to properly implement at some point.)

[fingolfin]

True, but this takes away the ability for other packages to implement this

Fair point.

[stertooy]

Made some changes:

• All new methods now require the IsFinite property to be set on any non-pcp argument.
• Removed the "promise" in the documentation that the output would always be a pcp-group.
• The example in the docs is now sets IsFinite to true manually, which should be helpful to users (I hope).
• Added the mixed infinite FpGroup - PcpGroup example to the tests, to ensure we don't replace that functionality.
• Clarified in the changelog that SemidirectProduct may now return a PcpGroup where it previously gave a PcGroup.

It was just too much trouble dealing with edge cases and higher ranks and so on, especially since GAP or another package could implement a new method with higher rank at any time...