Skip to content

Fix ElementaryAbelianSeries returning an invalid series after AutomorphismGroup - #6492

Merged
fingolfin merged 1 commit into
masterfrom
mh/fix-6407-special-pcgs-reuse
Aug 5, 2026
Merged

Fix ElementaryAbelianSeries returning an invalid series after AutomorphismGroup#6492
fingolfin merged 1 commit into
masterfrom
mh/fix-6407-special-pcgs-reuse

Conversation

@fingolfin

Copy link
Copy Markdown
Member

ElementaryAbelianSeries could return a series with a factor that is not elementary abelian, and ElementaryAbelianSeriesLargeSteps then looped forever on it:

H := Group([(1,2)(3,4)(5,8,7,6)(9,12)(10,11),
            (1,4,3,2)(5,7),
            (2,4)(5,6,7,8)(9,10,11,12)]);;
StabChain(H, [1,5,9]);;
FittingSubgroup(H);;
AutomorphismGroup(H);;
List(ElementaryAbelianSeries(H), Size);   # [ 64, 32, 16, 4, 2, 1 ] -- 16/4 is C4
ElementaryAbelianSeriesLargeSteps(H);     # hangs

All three preparatory calls are needed. FittingSubgroup leaves a central series pcgs on H which carries IndicesEANormalSteps, although its factors need not have exponent p — here one of them is a cyclic group of order 4. AutomorphismGroup then computes SpecialPcgs(H), and the generic method turns out to be handed that very pcgs. It has a guard for that case, which forces a fresh object so that the attributes it sets afterwards actually take effect, but the guard only tested LGWeights. So the pcgs was reused, the subsequent SetIndicesEANormalSteps was silently ignored, and the result was a pcgs marked IsSpecialPcgs whose IndicesEANormalSteps disagrees with its LGFirst. Through the implication IsSpecialPcgs => IsPcgsElementaryAbelianSeries the stale indices are then trusted without validation.

This PR widens that guard to cover all series indices the method sets, and makes ElementaryAbelianSeriesLargeSteps raise an error instead of spinning when it is handed a series that is not elementary abelian.

It deliberately does not touch the deeper cause, namely that TryPcgsPermGroup sets IndicesEANormalSteps on central pcgs at all (lib/pcgsperm.gi:423). That attribute is used as a generic "indices of the steps of this pcgs's series" in several places — conjugacy classes of p-groups read it off a central pcgs — so changing it needs a decision about the intended meaning of the attribute; that question is posted on the issue and still open. With this PR the containment that used to keep the situation harmless (IsPcgsElementaryAbelianSeries validating the stored indices before anyone trusts them) is restored.

Originally reported against CRISP (bh11/crisp#11), which only appeared in the traceback because CompositionSeriesUnderAction calls InvariantElementaryAbelianSeries right after AutomorphismGroup; it reproduces with gap -A and no packages.

Fixes #6407

AI disclosure: this change was prepared with the help of Claude Code (Claude Opus 5), which reduced the reproducer, diagnosed the cause and drafted the patch and the test; reviewed by me.

The generic `SpecialPcgs` method may end up with the very pcgs it was
handed, in which case it forces a fresh object so that the attributes it
subsequently sets actually take effect. That check only looked at
`LGWeights`, so a pcgs which already carried `IndicesEANormalSteps` (for
example one produced by `PcgsCentralSeries`, where the series is central
but not elementary abelian) was reused, and the following
`SetIndicesEANormalSteps` was silently ignored. The result was a pcgs
marked `IsSpecialPcgs` whose `IndicesEANormalSteps` disagreed with its
`LGFirst`; via the implication `IsSpecialPcgs => IsPcgsElementaryAbelianSeries`
this made `ElementaryAbelianSeries` return a series with a factor that is
not elementary abelian, and `ElementaryAbelianSeriesLargeSteps` then
looped forever on it.

Widen the check to cover all series indices the method sets, and make
`ElementaryAbelianSeriesLargeSteps` raise an error rather than spin when
the series it is given is not elementary abelian.

AI disclosure: this change was prepared with the help of Claude Code
(Claude Opus 5), which reduced the reproducer, diagnosed the cause and
drafted the patch and the test; reviewed by me.

Fixes #6407

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them topic: library labels Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.03%. Comparing base (c0ec89a) to head (1a34588).

Files with missing lines Patch % Lines
lib/grp.gi 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6492   +/-   ##
=======================================
  Coverage   79.03%   79.03%           
=======================================
  Files         685      685           
  Lines      293790   293801   +11     
  Branches     8664     8664           
=======================================
+ Hits       232203   232215   +12     
+ Misses      59786    59785    -1     
  Partials     1801     1801           

☔ 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.

@hulpke

hulpke commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This fixes this issue, but I wonder whether there is a more general problem
lurking in the background: (Yes, this is code which I built in many small
steps as need built up, sxo it is in some way my mess.)

When an algorithm requests a subnormal series with particular properties, it
also typically wants the indices in the series, that correspond to the
layers with these properties.
Originally, the idea was that IndicesNormalSteps would hold these indices
for a series. But the (sensible) re-using of series led to a proliferation
of such attributes, and now we need to be careful with what is carried
through.

I wonder whether the interface actually needs to be different:

Instead of asking for a series-exposing pcgs (and have the suitable indices
then stored in an attribute of the pcgs), should we have function(s) that
return a pcgs and a list of suitable indices. This does not prevent
caching of indices in the pcgs or a group caching multiple pcgs.

It would be an incompatible change, but the use of thes attributes outside
the library is probably small.

Would that make sense? Would this be many functions for different properties, or one function that gets properties specified?

@fingolfin

Copy link
Copy Markdown
Member Author

@hulpke that's a good point, and at least my first thought is that your idea is a good one. I'll ponder it a bit more (and let Claude also ponder it a bit more ;-). I'll also ask Claude to look through our packages to see how much these things are used there...

But for now, let's merge this quick fix.

@fingolfin
fingolfin merged commit 89eef0a into master Aug 5, 2026
33 checks passed
@fingolfin
fingolfin deleted the mh/fix-6407-special-pcgs-reuse branch August 5, 2026 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-4.16 kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them 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.

Slow CharacteristicSubgroups

2 participants