Improve conjugation test for subgroups with different orders

[limakzi]

Conjugate subgroups have the same order.
When the sizes of both subgroups are already known (via HasSize), IsConjugate and RepresentativeActionOp now return immediately if the sizes differ, avoiding expensive conjugacy computations.

Fixes #6266

[Copilot]

Pull request overview

Adds early-exit checks in IsConjugate and RepresentativeActionOp to immediately return when two subgroups have different known orders, avoiding expensive conjugacy computations.

Changes:

• Add size-comparison short-circuit in IsConjugate for groups (lib/grp.gi)
• Add size-comparison short-circuit in RepresentativeActionOp for permutation groups (lib/oprtperm.gi)
• Add a test case for the new behavior (tst/testinstall/ConjNatSym.tst)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
lib/grp.gi	Early return false when subgroup sizes are known and differ
lib/oprtperm.gi	Early return fail when subgroup sizes are known and differ
tst/testinstall/ConjNatSym.tst	Test case for non-conjugate subgroups with different orders

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[fingolfin]

I wonder if there are other properties and attributes (isomorphism invariants) that would be worthwhile to check? Like IsAbelian, IsSolvableGroup, IsPerfectGroup, NrConjugacyClasses and a gazillion more. With help of Tester and Getter one could easily handle them uniformly with a loop. But is it worth it? Hm.

I do think Size is worth it for sure

[limakzi]

I'm not sure. As you noted there are gazillion isomorphism invariants.
Some are difficult to compute and the difficulty differs between groups.
Proposal:

InstallGlobalFunction( KnownIsomorphismInvariantsMatch,
function( G, H )
    local inv;
    for inv in [ Size, AbelianInvariants, DerivedLength,
                 NilpotencyClassOfGroup, Exponent, HirschLength,
                 IsAbelian, IsNilpotentGroup, IsSolvableGroup,
                 IsPerfectGroup, IsSimpleGroup, IsSporadicSimpleGroup ] do
        if Tester(inv)(G) and Tester(inv)(H) and inv(G) <> inv(H) then
            return false;
        fi;
    od;
    return true;
end );

Then

if not KnownIsomorphismInvariantsMatch(d, e) then
  return fail;
fi;

[ThomasBreuer]

This IsConjugate method calls RepresentativeAction if the shortcut for normal subgroups does not strike. Thus I think it is sufficient to add new tests only in RepresentativeActionOp methods in question.

[limakzi]

Good! The only thing we miss is catalog of checks.
How do you like the proposal I put above?

[fingolfin]

Interesting: if one group is cyclic but the other isn't, that'd be also a reason to abort, which the code does not take.

Actually IsAbelian should be cheaper than IsCyclic, so we could just always test both of these.

[limakzi]

Well, another set of such invariants plus sorted list of orbits.

if SortedList(List(Orbits(d, MovedPoints(x)), Length)) <>
   SortedList(List(Orbits(e, MovedPoints(e)), Length)) then
  return fail;
fi;

[ThomasBreuer]

The IsCyclic tests are not intended for distinguishing the groups but for the choice of the algorithm.
My understanding of Max' comment is that we could use the tests for a fail result if the values are different. (But note that the calls are not reached if the group is reasonably small.)

Concerning a possible comparison of orbit lengths:
A few lines below there is the comment: "Do not test for same orbit lengths -- this will be done by next level routine".

[fingolfin]

yes, you understood me right: ny suggestion was indeed that beyond using the IsCyclic checks for selecting an algorithm, one can also use them to handle some "trivial" cases.

[lgoettgens]

please add a test that uses huge groups (as the one in #6266), so that it needs to use the new shortcut. The test that you added here will already work right now, and will also be quite fast due to the size of the groups.

[ThomasBreuer]

If the first argument of IsConjugate knows that it is a natural symmetric group then none of the methods affected by this pull request gets called.

[limakzi]

@ThomasBreuer Do you have proposal for group?
I do not like the idea of loading package from the #6266, for instance.

[ThomasBreuer]

gap> G:= SymmetricGroup( 1000 );;
gap> H:= Subgroup( G, [ G.1 ] );; Size( H );
1000
gap> K:= Subgroup( G, [ G.2 ] );;  Size( K );
2
gap> G:= Group( GeneratorsOfGroup( G ) );;
gap> IsConjugate( G, H, K );
false

Creating the group again is needed in order to achieve that the special method for symmetric groups does not get called. Additionally, the effect is that the new group does not store its Size, which means that the example needs a long time without this pull request: This is because of the Size( G ) call that occurs below the new test whether the two subgroups have the same Size value.