Skip to content

Add PcgsSeriesInfo returning a pcgs together with the indices of its series - #6503

Draft
fingolfin wants to merge 1 commit into
masterfrom
mh/pcgs-series-info
Draft

Add PcgsSeriesInfo returning a pcgs together with the indices of its series#6503
fingolfin wants to merge 1 commit into
masterfrom
mh/pcgs-series-info

Conversation

@fingolfin

Copy link
Copy Markdown
Member

Following the discussion with @hulpke in #6492: the indices of the normal series a pcgs refines are stored on the pcgs, as IndicesEANormalSteps and friends. That has three consequences, and this PR addresses the first one.

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 — ClassesSolvableGroup, MultiClassIdsPc, CentralizerSolvableGroup and CentralizerModulo, each in its IsPGroup branch. That is why TryPcgsPermGroup has 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:

info := PcgsCentralSeriesInfo( G );                    # rec( pcgs := …, indices := … )
info := PcgsSeriesInfo( IsPcgsCentralSeries, G );      # the same, dispatched 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. IndicesEANormalStepsBounded gains a variant IndicesNormalStepsBounded( 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:423 in 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 (TryPcgsPermGroup for 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.

DeclareTagBasedOperation rather 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 returns rec( radical, pcgs, depths, pcisom, … ) and is the one corner of this machinery that has never had this class of bug; BoundedRefinementEANormalSeries likewise takes the indices as an argument instead of consulting attributes.

Open questions for reviewers

  • Names. PcgsPCentralSeriesPGroupInfo is a mouthful, and Info may be too vague — …SeriesSetup, echoing FittingFreeLiftSetup, is the other obvious candidate. Happy to rename to whatever finds consensus.
  • Whether to split. The call-site conversion alone is uncontroversial and is what unblocks the follow-up; the new API is the part that needs design agreement. If preferred, I can split this into two PRs with the conversion first, using IndicesCentralNormalSteps etc. directly and no new API.
  • Should the tagless default method error, as it does here, or fall back to computing a series and verifying the property? Tags match by identity, so asking for IsPcgsElementaryAbelianSeries will not be answered with a p-central series even though that would qualify.

Two observations from writing this, relevant to the wider discussion: IsPcgsCentralSeries, IsPcgsPCentralSeriesPGroup and IsPcgsChiefSeries have no computing methods at all — they are stamps which nothing can verify, unlike IsPcgsElementaryAbelianSeries. And the current answers are order dependent: asking SmallGroup(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 html and testmanuals, the converted code paths were compared against the unmodified library on conjugacy classes, centralizer orders, rational classes and CentralizerModulo for 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.

@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes kind: new feature topic: library labels Aug 12, 2026
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.41667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.06%. Comparing base (104ccfa) to head (aef1eb4).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
lib/claspcgs.gi 50.00% 7 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`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
fingolfin marked this pull request as draft August 12, 2026 12:48
@fingolfin

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: new feature release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant