Speed up SchurCover, EpimorphismSchurCover and AbelianInvariantsMultiplier - #6485
Open
fingolfin wants to merge 9 commits into
Open
Speed up SchurCover, EpimorphismSchurCover and AbelianInvariantsMultiplier#6485fingolfin wants to merge 9 commits into
SchurCover, EpimorphismSchurCover and AbelianInvariantsMultiplier#6485fingolfin wants to merge 9 commits into
Conversation
fingolfin
added
kind: enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6485 +/- ##
==========================================
- Coverage 79.08% 79.07% -0.02%
==========================================
Files 685 685
Lines 293789 293841 +52
Branches 8664 8641 -23
==========================================
+ Hits 232357 232359 +2
- Misses 59631 59679 +48
- Partials 1801 1803 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
limakzi
reviewed
Aug 5, 2026
limakzi
approved these changes
Aug 5, 2026
fingolfin
changed the title
SchurCover, EpimorphismSchurCover and AbelianInvariantsMultiplier
The efficient methods for EpimorphismSchurCover and AbelianInvariantsMultiplier were disabled in 2005 because they could return wrong results. The cause is in the fusion relations which cut the multiplier of a Sylow p-subgroup P down to the p-part of the multiplier of G: they were evaluated only for elements in the image of the corestriction from P cap P^g, so the relations arising from its kernel were lost and the multiplier could come out too large. The smallest example is SmallGroup(48,30). The relations are now read off from a presentation of P cap P^g, which accounts for all of its second homology. Schur covers of p-groups are computed by applying the p-quotient algorithm to the cover of a power-commutator presentation. This returns a pc group and avoids both Tietze transformations and coset enumeration, which makes it usable for p-groups where the previous method ran out of cosets. Also correct the documentation of EpimorphismSchurCover and SchurCover, which claimed that the cover is always a finitely presented group. Fixes gap-system#5294 Claude Code (Opus 5) was used to diagnose the defect, write the code, documentation and tests, and run the validation against the previous method and against the cohomolo package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three changes, none of which affect the computed values. SchuMu now does its work inside a pc group isomorphic to the Sylow subgroup. Building the power-commutator presentations needed for the fusion relations costs about 6ms per subgroup in a matrix group but 0.35ms in a pc group, and one presentation is built per double coset representative. The p-quotient in EpimorphismSchurCoverPGroup is preceded by a Tietze simplification of the cover presentation, which SchurCoverFP leaves with one generator per relator. The simplification pays for itself several times over. AbelianInvariantsMultiplier passes matrix groups to a permutation image first, since the Sylow subgroup, normalizer and double coset computations delegate to one anyway; doing so once rather than per operation halves the runtime. Intersections of Sylow subgroups which are cyclic are skipped, as their multiplier is trivial. For AbelianInvariantsMultiplier this gives, for example, 715ms to 230ms for Sp(6,2) and 11.0s to 4.6s for Sp(8,2). Claude Code (Opus 5) was used to profile the code, implement the changes and re-run the validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CorestEval evaluates the relators of a presentation of G over a transversal of a Sylow subgroup, and did so by multiplying in G and locating the result in the transversal for every letter of every relator. It now tabulates, per generator and inverse, the induced permutation of the transversal together with the preimage of the resulting Schreier element, so the relator loop computes only in the covering group of the Sylow subgroup. Building the tables costs the number of generators times the index, the loop they replace the index times the total relator length. For Sp(6,2) this takes CorestEval from 14.3s to 2.2s. Matrix groups are passed to a permutation image, as for AbelianInvariantsMultiplier, and the resulting epimorphism transported back; the transversal arithmetic is far cheaper there. MulExt verified the order of the cover whenever assertions were on, but that means enumerating the cosets of its trivial subgroup, which costs much more than the whole computation. As START_TEST sets the assertion level to 2, this made the operation unusable from the test suite. The check now happens only above that level. Together this takes EpimorphismSchurCover for Sp(6,2) from over ten minutes to 1.6s. Claude Code (Opus 5) was used to profile the code, implement the changes and re-run the validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tions The Sylow subgroup based methods were installed for all finite groups with a method rank chosen so that more specific methods, in particular those of the polycyclic package for pcp groups, would still be preferred. That relied on the numerical difference between two filter ranks. Install them instead for the representations providing the Sylow subgroup, normalizer and double coset computations they need, namely permutation groups, pc groups, and finite groups handled by a nice monomorphism. Groups with their own methods are then simply not covered, so all ranks can be zero and the generic fallbacks no longer need a negative one. As a side effect, every group working through a nice monomorphism now takes the detour via a permutation image which so far only matrix groups took. Claude Code (Opus 5) was used to implement the change and re-run the validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both claimed that the cover is a finitely presented group. It is one for a general finite group, but not for p-groups, natural symmetric and alternating groups, or pcp groups. Say which representation arises when, and drop the suggestion to apply IsomorphismFpGroup to the result, which was only there because no way to request a presentation exists. Also state that SchurCover is the source of EpimorphismSchurCover, which holds for every group except a finitely presented one, where a separate method returns an unsimplified cover instead. Claude Code (Opus 5) was used to check the behaviour and write the text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Cover For a finitely presented group SchurCover applied SchurCoverFP directly, bypassing EpimorphismSchurCover and returning a cover with one generator per relator of the input presentation. Every other group got the source of EpimorphismSchurCover, so the two entry points yielded different, and for fp groups needlessly large, covers. Remove the method, leaving the generic one. Finitely presented groups now get the Tietze simplified cover the epimorphism has always been built from. Claude Code (Opus 5) was used to implement the change and re-run the validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tst/testinstall is also run with --bare, where no packages are available, so SmallGroup cannot be used there. Construct the groups directly instead: SmallGroup(48,30) as the subgroup of S4 x C4 of index two, SmallGroup(48,48) as a permutation group, and use DihedralGroup for the p-group cases. Avoid DirectProduct of a pc group and a permutation group as well, as IsomorphismFpGroup has no method for the result when running bare. Claude Code (Opus 5) was used to make the change and verify it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ThomasBreuer
approved these changes
Aug 14, 2026
ThomasBreuer
left a comment
ThomasBreuer
left a comment
Contributor
There was a problem hiding this comment.
Looks impressive.
If I understand the changes right then EpimorphismSchurCover( PerfectGroup( IsFpGroup, 60, 1 ), [ 2 ] ) worked before the changes and signals an error afterwards. If this is intended then the documentation for the 2-argument version of EpimorphismSchurCover should mention this restriction.
Concerning Assert calls that are deliberately not executed in testfiles:
Technically, the danger is that the code in the Assert statement is itself wrong, and if it does not get tested then this will not be detected.
Logically, if the Assert statement is meaningful at all then there should be situations where it gets executed.
There are other places in the library where executing Assert statements can be very expensive in larger examples, and the testfiles are carefully running only small examples. Would it perhaps be sufficient to change the level temporarily inside testfiles, such that small examples run the test in the Assert statement, and larger examples don't?
hulpke
approved these changes
Aug 14, 2026
hulpke
left a comment
hulpke
left a comment
Contributor
There was a problem hiding this comment.
I'm delighted that someone went through this old code of mine and found bugs. As I had been unable to diagnose it (I had suspected inconsistency in choice of repeated PreImages), I am reluctant to say it will fix all bugs but certainly is better than before. Thanks!
The two argument form raised an error for finitely presented groups on the assumption that they lack the Sylow subgroup machinery MulExt needs. They do not, and it worked before, so let it through again. Claude Code (Opus 5) was used to make the change and verify it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MulExt checked the order of the cover whenever assertions were on. That check was moved above the level START_TEST sets, which left it never executed by the test suite, and an assertion that never runs is not worth having. Restore it and instead lower the assertion level around the one example where it is expensive: for Sp(6,2) the check enumerates all 2903040 cosets of the trivial subgroup of the cover, taking minutes against 1.2s for the cover itself. Also spell the symplectic group Sp rather than SP. Claude Code (Opus 5) was used to make the change and verify it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fingolfin
force-pushed
the
claude/gap-schur-cover-methods-5401c5
branch
from
August 16, 2026 07:34
9077a5d to
d4c0e84
Compare
ThomasBreuer
approved these changes
Aug 16, 2026
ThomasBreuer
left a comment
ThomasBreuer
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the latest changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Sylow subgroup based methods for these operations were disabled in 2005 because they could return a multiplier that was too large.
SchuMuderived its fusion relations by writing elements ofM(P)as commutator words, which only reachesyin the image ofcor^P_Q; the relations coming from its kernel were lost. They are now read off from a presentation ofQ, which accounts for all ofH_2(Q).SmallGroup(48,30)is the smallest group affected.With the methods available again, plus a pc group cover for p-groups, a tabulated coset action in
CorestEval, and a single detour to a permutation image rather than one per subgroup computation:AbelianInvariantsMultiplier(MathieuGroup(11))AbelianInvariantsMultiplier(MathieuGroup(22))EpimorphismSchurCover(Sp(6,2))cohomoloneeds 394ms for the multiplier ofSp(6,2).Covers of p-groups are returned as pc groups, and
SchurCoverof a finitely presented group is now the simplified cover thatEpimorphismSchurCoverwas already built from, instead of one with a generator per relator.SchurCover(G)isSource(EpimorphismSchurCover(G))for every group.Fixes #5294, which reports that the documentation claims the cover is always finitely presented.
Validated against the previous method on all 2769 groups of prime power order up to 243 and on all groups of order up to 100, against
cohomoloin 760 cases, and against the ATLAS for eleven simple groups.AI disclosure
Claude Code (Opus 5) diagnosed the defect, wrote the code, documentation and tests, and ran the validation.
🤖 Generated with Claude Code