Add PcgsSeriesInfo returning a pcgs together with the indices of its series - #6503
Draft
fingolfin wants to merge 1 commit into
Draft
Add PcgsSeriesInfo returning a pcgs together with the indices of its series#6503fingolfin wants to merge 1 commit into
PcgsSeriesInfo returning a pcgs together with the indices of its series#6503fingolfin wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6503 +/- ##
==========================================
+ Coverage 79.01% 79.06% +0.05%
==========================================
Files 685 685
Lines 294022 293871 -151
Branches 8666 8664 -2
==========================================
+ Hits 232328 232356 +28
+ Misses 59895 59713 -182
- Partials 1799 1802 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`IndicesEANormalSteps' is in practice used as "the indices of whichever series this pcgs belongs to": four places in the library read it off a central or p-central pcgs. That is why `TryPcgsPermGroup' has to set it for central series as well, although the factors of a central series need not be elementary abelian -- which is how the inconsistent pcgs behind issue #6407 could arise in the first place. Add an attribute per kind of series which returns the pcgs together with the indices belonging to that kind, and a tag based operation dispatching on the kind, so that a caller obtains the indices it asked for rather than whatever the pcgs happens to carry. Use these in the four places above, and give `IndicesEANormalStepsBounded' a variant which takes the indices as an argument instead of reading them off the pcgs. This does not yet change what is stamped onto a central pcgs; it removes the library's dependence on that stamp, which is a prerequisite for doing so. AI disclosure: this change was prepared with the help of Claude Code (Claude Opus 5), which surveyed the affected call sites, drafted the patch, the documentation and the test, and checked the conversion against the previous behaviour; reviewed by me. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fingolfin
marked this pull request as draft
August 12, 2026 12:48
Member
Author
|
I've not yet reviewed this AI generated PR myself. So you may wish to hold off a review until I did so to avoid potentially being exposed to slop. |
fingolfin
force-pushed
the
mh/pcgs-series-info
branch
from
August 13, 2026 23:49
513e5b8 to
aef1eb4
Compare
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
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.
Following the discussion with @hulpke in #6492: the indices of the normal series a pcgs refines are stored on the pcgs, as
IndicesEANormalStepsand friends. That has three consequences, and this PR addresses the first one.IndicesEANormalStepsis in practice used as "the indices of whichever series this pcgs belongs to". Four places in the library read it off a central or p-central pcgs —ClassesSolvableGroup,MultiClassIdsPc,CentralizerSolvableGroupandCentralizerModulo, each in itsIsPGroupbranch. That is whyTryPcgsPermGrouphas to set the attribute for central series too (lib/pcgsperm.gi:423,(elab=true) or cent), although the factors of a central series need not be elementary abelian — which is how the inconsistent pcgs behind #6407 could arise in the first place.This PR adds an attribute per kind of series which returns the pcgs together with the indices belonging to that kind, and a tag based operation dispatching on the kind:
and uses them in the four places above, so that a caller obtains the indices of the series it asked for rather than whatever the pcgs happens to carry.
IndicesEANormalStepsBoundedgains a variantIndicesNormalStepsBounded( pcgs, indices, bound )taking the indices as an argument, because two of those call sites pass their pcgs on to it.Nothing is removed or deprecated here, and what gets stamped onto a central pcgs is unchanged. The point is to remove the library's dependence on that stamp, which is the prerequisite for changing
lib/pcgsperm.gi:423in a follow-up.On the design
The work sits in one ordinary attribute per kind rather than in the dispatching operation, because the kind is not the axis that decides the implementation — the group is (
TryPcgsPermGroupfor permutation groups, the LG series for pc groups, nice monomorphism otherwise). Ordinary attributes dispatch and rank on the group, and cache; the tag based operation sits on top and only delegates.DeclareTagBasedOperationrather than a constructor, because with a constructor only the rank of the first argument counts, so methods differing in what they require of the group rank equal and installation order decides which is chosen. A tag based operation has exactly one applicable method per tag, permits a default method, and leaves the remaining arguments alone.Returning a record follows
FittingFreeLiftSetup, which returnsrec( radical, pcgs, depths, pcisom, … )and is the one corner of this machinery that has never had this class of bug;BoundedRefinementEANormalSerieslikewise takes the indices as an argument instead of consulting attributes.Open questions for reviewers
PcgsPCentralSeriesPGroupInfois a mouthful, andInfomay be too vague —…SeriesSetup, echoingFittingFreeLiftSetup, is the other obvious candidate. Happy to rename to whatever finds consensus.IndicesCentralNormalStepsetc. directly and no new API.IsPcgsElementaryAbelianSerieswill not be answered with a p-central series even though that would qualify.Two observations from writing this, relevant to the wider discussion:
IsPcgsCentralSeries,IsPcgsPCentralSeriesPGroupandIsPcgsChiefSerieshave no computing methods at all — they are stamps which nothing can verify, unlikeIsPcgsElementaryAbelianSeries. And the current answers are order dependent: askingSmallGroup(8,3)for its central series first makes the later elementary abelian query return[ 1, 3, 4 ], asking for the elementary abelian one first returns[ 1, 2, 3, 4 ]. Both are valid elementary abelian series, but it shows the reuse-and-stamp mechanics in the open.Testing
Beyond
testinstall,teststandard,testbugfix,make htmlandtestmanuals, the converted code paths were compared against the unmodified library on conjugacy classes, centralizer orders, rational classes andCentralizerModulofor every group of order 16, 27, 32, 64, 81, 96 and 128, plus several permutation p-groups and a direct product: 11421 results, identical.Context: #6407, discussion in #6492.
AI disclosure: this change was prepared with the help of Claude Code (Claude Opus 5), which surveyed the affected call sites, drafted the patch, the documentation and the test, and checked the conversion against the previous behaviour; reviewed by me.