Speed up AutomorphismGroup for solvable Frattini-free groups - #6500
Conversation
fingolfin
added
release notes: use title
|
Seems sensible, but unfortunately |
Presumably the issue is that the output is not marked as automorphism group, and so no methods apply. |
The failure is a bit misleading, the error happens already in The respective call stack is |
For a solvable group, AutomorphismGroup checked HasIsFrattiniFree before using AutomorphismGroupFrattFreeGroup, so that path was taken only for groups that had the property set beforehand, in practice only those coming from the group construction and random isomorphism code. The reason was that IsFrattiniFree had no method to compute it, so asking for it would have run into an error. Now that IsFrattiniFree is computable, ask for it directly. Deciding it is cheap compared to constructing the automorphism group, and for the groups it applies to the resulting speedup is a factor of two to six; for groups which are not Frattini-free the extra test does not measurably cost anything. AutomorphismGroupFrattFreeGroup needs a pcgs not just for the group itself but also for a complement of its Fitting subgroup, which for example is not available for finitely presented groups. It is therefore only used for groups that can easily compute a pcgs, which covers the pc groups and solvable permutation groups the speedup was measured for. Note that grppcext.gi guards a similar use of the property, in order to decide whether reducing a set of extensions is worth it. There the check whether the property is already known is a deliberate cost heuristic, alongside a HasAutomorphismGroup check, so it is left alone. Prepared with the help of Claude Code (Opus 5), which made the change, cross-checked the results against the previous code path and against AutomorphismGroupMorpheus, and ran the test suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7df4b04 to
6b2b972
Compare
|
Fix committed |
3 participants
Follow-up to #6487, implementing what @hulpke pointed out there:
For a solvable group,
AutomorphismGrouptestedHasIsFrattiniFree(G) and IsFrattiniFree(G)before usingAutomorphismGroupFrattFreeGroup, so that path was only ever taken for groups which had the property set beforehand -- in practice only those produced by the group construction and random isomorphism code. Now thatIsFrattiniFreecan be computed, the property is simply asked for.Deciding
IsFrattiniFreeis cheap compared to constructing the automorphism group. Timings for the completeAutomorphismGroupcall, so the cost of the new test is included in the "new" column:SmallGroup(864,4675)SmallGroup(600,148)WreathProduct(SymmetricGroup(3),SymmetricGroup(3))SmallGroup(1080,497)DihedralGroup(100)(not Frattini-free)Note that
AutomorphismGroupFrattFreeGroupwas previously not reached by the test suite at all, and that the new route also applies to permutation and matrix groups, which the old one essentially never saw. It therefore seemed prudent to check it more broadly than the test suite does: for every solvable, non-nilpotent, Frattini-free group of order at most 200 the resulting automorphism group order was compared with the one obtained fromAutomorphismGroupSolvableGroup, and for order at most 100 also with the one from the independentAutomorphismGroupMorpheus; the same was done for permutation representations of these groups. This covered 340 pc groups and 124 permutation groups without a single mismatch. The added test inAutomorphismGroup.tstmakes sure the code path is exercised by CI from now on.grppcext.giguards a similar use of the property when deciding whether reducing a set of extensions is worthwhile. There the test whether the property is already known looks like a deliberate cost heuristic -- it sits next to aHasAutomorphismGroupcheck -- so it is left alone here; happy to revisit if that is wrong.AI disclosure: prepared with the help of Claude Code (Opus 5), which made the change, ran the cross-checks and test suites described above, and drafted this description.