Skip to content

Fix and document argument handling of Blocks, Cycle, Permutation and related functions; Blocks and RepresentativesMinimalBlocks now reject intransitive actions - #6520

Merged
ThomasBreuer merged 7 commits into
masterfrom
claude/permutation-groups-docs-usability-98a248
Aug 25, 2026
Merged

Conversation

@fingolfin

Copy link
Copy Markdown
Member
  • Fixes Documentation of ActionHomomorphism is misleading #3484: ActionHomomorphism: "this homomorphism is a permutation equivalence" reads as a claim about the returned map, which is false for unfaithful actions. Name the actual identification of Omega with [1..|Omega|] instead.
  • Fixes Clarify documentation for MaximalBlocks #4700: MaximalBlocks: "blocks are maximal with respect to inclusion" also describes [ Omega ]. Say maximal among the proper subsets of Omega, i.e. the induced action is primitive, and what happens for a primitive action.
  • Fixes Improve argument checking in Cycles and other related functions #3720: Permutation, PermutationCycle, Cycle, Cycles indexed arg unguarded, so Cycles(g) died with List Element: <list>[2] must have an assigned value. Adds usage guards, and documents the gens, acts forms, the external set forms, and one argument CycleIndex, all previously implemented but undocumented. Also fixes a recursion trap in the gens, acts form of Cycle and CycleLength, which built the action homomorphism from an external orbit rather than an external set.
  • Fixes Error message for Blocks is incorrect #3541: OrbitishFO used one hard coded usage message for every operation it creates; the Blocks family may now pass its own. Also Blocks(<G>,1) reports the usage instead of "no method found for BlocksOp on 5 arguments".
  • Fixes Blocks should either work for intransitive actions or consistently reject them #3364: BlocksOp returned [ Omega ] for a domain of prime size before checking transitivity, so an intransitive group either errored or returned nonsense depending on the degree. Check first, take the shortcut second; same in RepresentativesMinimalBlocksOp; add the check to the seeded BlocksOp method, which had none.

Behaviour change: Blocks and friends now raise an error in cases where they used to return a meaningless result. The two spellings of that error message now agree ("must act transitively").

Prepared with AI assistance (Claude).

fingolfin and others added 5 commits August 24, 2026 00:30
The text said that "this homomorphism is a permutation equivalence",
which reads as a claim about the returned map; that claim is wrong as
soon as the action is unfaithful.  What is meant is the identification
of Omega with [ 1 .. |Omega| ] used to express the images.

Fixes #3484

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Blocks are maximal with respect to inclusion" could be read as
describing the system [ Omega ], which is not what MaximalBlocks
returns.  Spell out that the blocks are maximal among the proper
subsets of Omega, i.e. that the induced action is primitive, and say
what happens for a primitive action.  State the analogous degenerate
case for Blocks as well.

Fixes #4700

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Permutation, PermutationCycle, Cycle and Cycles indexed their argument
list without checking its length, so a call with too few arguments died
with "List Element: <list>[2] must have an assigned value" instead of a
usage message.  CycleLength and CycleLengths only checked a lower bound.

The <gens>, <acts> forms of Cycle and CycleLength ran into a recursion
trap because they built the action homomorphism from an external orbit;
use the same external set as the neighbouring functions.

Document the <gens>, <acts> forms and the external set form of the
whole family, and the one argument form of CycleIndex; all of these
were implemented but undocumented.

Fixes #3720

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OrbitishFO used one hard coded usage message for every operation it
creates.  For the Blocks family that message is wrong: the third
argument is an optional list <seed>, not a mandatory point <pnt>, and
Blocks(<xset>) is a legal call.  Let callers pass their own message.

Also reject a <pnt> that no method of the underlying operation can
accept, so that Blocks(<G>,1) reports the usage instead of dying with
"no method found for BlocksOp on 5 arguments".

Fixes #3541

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BlocksOp for permutation groups returned the block system [ Omega ] for
a domain of prime size before it checked transitivity, so the same
intransitive group either errored or silently returned nonsense,
depending only on the degree:

    gap> Blocks( Group( (1,2,3,4) ), [1..5] );
    [ [ 1 .. 5 ] ]
    gap> Blocks( Group( (1,2,3,4) ), [1..4] );
    Error, <G> must operate transitively on <D>

Check transitivity first, and only then take the shortcut for prime
degree.  Do the same in RepresentativesMinimalBlocksOp, and add the
check to the seeded BlocksOp method, which had none at all.

The two error messages used for this now agree.

Fixes #3364

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fingolfin fingolfin added topic: documentation Issues and PRs related to documentation topic: library labels Aug 23, 2026
@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.00%. Comparing base (e0bc786) to head (242ccc1).

Files with missing lines Patch % Lines
lib/ghomperm.gi 87.50% 1 Missing ⚠️
lib/oprt.gd 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6520      +/-   ##
==========================================
+ Coverage   78.96%   79.00%   +0.03%     
==========================================
  Files         685      685              
  Lines      294270   294310      +40     
  Branches     8669     8669              
==========================================
+ Hits       232364   232508     +144     
+ Misses      60102    59996     -106     
- Partials     1804     1806       +2     

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

@ThomasBreuer

Copy link
Copy Markdown
Contributor

The following example from the just closed #4904 is not yet addressed, a fix would fit into the current pull request.

gap> g:= Group( [ (2,4,3), (3,4) ] );
Group([ (2,4,3), (3,4) ])
gap> hom:= ActionHomomorphism( g, [ 3, 4 ], OnPoints );
<action homomorphism>
gap> MappingGeneratorsImages( hom );
[ [ (2,4,3), (3,4) ], [ fail, (1,2) ] ]
gap> Image( hom );
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MagmaWithInversesByGenerators' on 1 arguments
[...]

@fingolfin fingolfin changed the title Five small fixes to documentation, error messages and argument checking around group actions Clarify documentation and error messages of ActionHomomorphism, Blocks, Cycle, CycleLength, Cycles, MaximalBlocks, Permutation, PermutationCycle; and catch some invalid inputs early Aug 24, 2026
@fingolfin fingolfin added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Aug 24, 2026
@fingolfin fingolfin changed the title Clarify documentation and error messages of ActionHomomorphism, Blocks, Cycle, CycleLength, Cycles, MaximalBlocks, Permutation, PermutationCycle; and catch some invalid inputs early Clarify documentation and error messages of ActionHomomorphism, Blocks, Cycle, CycleLength, Cycles, MaximalBlocks, Permutation, PermutationCycle; also catch and reject invalid inputs early, instead of returning garbage Aug 24, 2026
@fingolfin fingolfin changed the title Clarify documentation and error messages of ActionHomomorphism, Blocks, Cycle, CycleLength, Cycles, MaximalBlocks, Permutation, PermutationCycle; also catch and reject invalid inputs early, instead of returning garbage Fix and document argument handling of Blocks, Cycle, Permutation and related functions; Blocks and RepresentativesMinimalBlocks now reject intransitive actions Aug 24, 2026
@fingolfin
fingolfin marked this pull request as ready for review August 24, 2026 08:00
For an action on a subset of the moved points, ImagesRepresentative
dispatches to the constituent homomorphism method, which handed the
`fail' of PermList to its caller.  That `fail' then surfaced far from
its cause:

    gap> hom:= ActionHomomorphism( Group([(2,4,3),(3,4)]), [3,4] );;
    gap> MappingGeneratorsImages( hom );
    [ [ (2,4,3), (3,4) ], [ fail, (1,2) ] ]
    gap> Image( hom );
    Error, no method found! ...
    Error, no 1st choice method found for `MagmaWithInversesByGenerators'
    on 1 arguments

Signal an error instead, as ImageElmActionHomomorphism already does for
the actions it handles, and honour the `actioncanfail' option, which
the nice monomorphism code relies on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@ThomasBreuer ThomasBreuer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks.

The method for a given seed computes no orbit of its own, so unlike the
method without seed it cannot get transitivity for free.  The generic
`Orbit' used for the check dominated the whole computation, by a factor
of up to 9 for large degrees; `OrbitPerms' brings that down to 20-50%,
and is applicable because the method already requires the acting
elements to be permutations and <act> to be OnPoints.

For callers that know the action is transitive, the option
`check := false' switches the test off entirely.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread lib/oprtperm.gi
Comment on lines +447 to +449
if ValueOption( "check" ) <> false and Length( D ) > 1
and ( Length( acts ) = 0
or Length( OrbitPerms( acts, D[1] ) ) <> Length( D ) ) then

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomasBreuer you were completely right to question the use of Orbit here before; it was in fact measurably impacting performance (negatively, of course).

But with OrbitPerms called directly, the overhead is very small; and in addition it can now be turned off by passing check:=false as a global option.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@ThomasBreuer
ThomasBreuer merged commit fecc9af into master Aug 25, 2026
33 checks passed
@ThomasBreuer
ThomasBreuer deleted the claude/permutation-groups-docs-usability-98a248 branch August 25, 2026 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: documentation Issues and PRs related to documentation topic: library

Projects

None yet

2 participants