support multiple `NrMovedPoints` arguments in `PrimitiveGroupsIterator` by ThomasBreuer · Pull Request #82 · gap-packages/primgrp

ThomasBreuer · 2026-09-01T15:41:57Z

addresses #80

(Fortunately, NrMovedPoints was the only function that had this problem.)

codecov · 2026-09-01T15:57:49Z

Codecov Report

❌ Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.19%. Comparing base (b279b65) to head (0013f93).

Files with missing lines	Patch %	Lines
lib/primitiv.gi	94.11%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage   99.13%   99.19%   +0.06%     
==========================================
  Files          46       46              
  Lines       38327    38333       +6     
==========================================
+ Hits        37995    38026      +31     
+ Misses        332      307      -25

Files with missing lines	Coverage Δ
lib/primitiv.gi	`64.48% <94.11%> (+4.72%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ThomasBreuer · 2026-09-01T16:22:58Z

Concerning code coverage:

If we want to see the warnings about missing degree restrictions, then we have to call PrimitiveGroupsIterator in situations where the arguments do not yield a degree restriction inside PRIMRANGE.
This means that a search over the whole library is executed, which is at least time-consuming.

Moreover, one gets an error message if the library of groups of degree larger than 4095 is not installed.
Can we assume that the groups of these degrees are available in the tests?

Turning this question around:
Would it perhaps be useful to adjust PRIMRANGE to the two possible situations that either only the small degree groups are available or also the large degree groups?

fingolfin · 2026-09-03T11:54:23Z

We will soon have all groups of degree < 8192 in this repo, so part of your question will become moot. I need to find some time to properly think about the rest.

BTW I think this also affects Size. E.g:

gap> List(AllIrreducibleSolvableGroups(Dimension,2,Characteristic,3,Size,8,Size,16),Size);
[ 16 ]
gap> List(AllIrreducibleSolvableGroups(Dimension,2,Characteristic,3,Size,16,Size,8),Size);
[ 8, 8, 8 ]

ThomasBreuer · 2026-09-03T12:50:24Z

BTW I think this also affects Size. E.g:

But this is about another function, AllIrreducibleSolvableGroups.
I was not aware of its existence.
(Well, I knew about such a function in the IrredSol package, apparently its name is AllIrreducibleSolvableMatrixGroups.)

ThomasBreuer · 2026-09-03T13:30:05Z

Trying to understand the background of AllIrreducibleSolvableGroups, I am getting even more irritated.
Apparently the IrredSol package provides a database of irreducible solvable matrix groups, and the PrimGrp package provides a small subset of this database.
The IrredSol manual mentions this fact, and provides functions for translating between the two sources where this makes sense.
The PrimGrp manual does not mention IrredSol at all. In fact, the manual chapter about Irreducible Matrix Groups is not even mentioned in the package overview.
(Well, currently just the Abstract of the manual would be a sensible place for that.)

fingolfin

Thanks a lot for working on this. A few thoughts..

fingolfin · 2026-09-07T14:26:28Z

I have this and a couple other changes already worked out, and will update the PR, @ThomasBreuer ; let me know if that's a nuisance, then I can also revert it

Both sides add tst/testinstall/interface.tst; keep the checks from either one, and declare `d` alongside `grps`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Fold the `NrMovedPoints` conditions into `deg` as they are scanned, instead of first sorting them into two auxiliary lists. Rename `f` to `unrestrictedDegree` and drop the assignment that the initialisation above now covers. Only the odd argument positions are scanned for `NrMovedPoints`, so a value that happens to equal it can no longer be read as a condition. The warning is now derived per condition rather than from the intersection of all of them. It stays exact whenever some condition lies in `PRIMRANGE`; it warns needlessly only when every requested set reaches outside the library but their intersection does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A primitive group is transitive, so its degree divides its order. The resulting degree restriction was derived from the first `Size` or `Order` condition only, and was dropped entirely whenever the degree was also named explicitly. Deriving it from all of them narrows the search instead: AllPrimitiveGroups( NrMovedPoints, [1..2000], Size, 168 ) now inspects the 16 divisors of 168 rather than 2000 degrees, 149 ms down to 1 ms. The answers are unaffected either way, since every condition is applied again while the groups are selected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fingolfin

Looks good to me but since I made recent changes with help of Claude, I will not merge this unless @ThomasBreuer also agrees it is OK (and he might very well not be OK with it)

ThomasBreuer · 2026-09-08T07:28:28Z

    if IsList(p) then
-      f:=not IsSubset(deg,Difference(p,[1]));
-      deg:=Intersection(deg,p);
+      unrestrictedDegree:= unrestrictedDegree and not IsSubset(PRIMRANGE, p);


No.
We cannot decide unrestrictedDegree from the individual degree lists, only their intersection gives the answer. We do not want the warning in the following example.

gap> PRIMRANGE; [ 1 .. 8191 ] gap> PrimitiveGroupsIterator( NrMovedPoints, [ 1, 3 .. 8193 ], NrMovedPoints, [ 2, 4 .. 8192 ] );; #W AllPrimitiveGroups: Degree restricted to [ 2 .. 8191 ]

You are absolutely right. Code rewritten now.

Thanks.
This looks good now.

A degree list reaching beyond PRIMRANGE does not by itself mean the library missed something: the search is complete as soon as the degrees all the conditions agree on lie inside it. Deciding per condition warned needlessly, e.g. for NrMovedPoints, [ 1, 3 .. 8193 ], NrMovedPoints, [ 2, 4 .. 8192 ] whose intersection is empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`deg` is the file's name for the degrees being searched, in a dozen functions; `degrees` next to it read as its plural rather than as the degrees the caller asked for. Name the requests after what they are, and pair them: `requestedDegrees`, `requestedSizes`. `unrestrictedDegree` named the wrong thing. It is set for `NrMovedPoints, 9000`, where the degree is restricted, just not to something the library holds. What it says is that the answer may miss groups, which is why a warning follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

support multiple NrMovedPoints arguments in PrimitiveGroupsIterator

b7ebabf

fingolfin reviewed Sep 7, 2026

View reviewed changes

Comment thread lib/primitiv.gi Outdated

Comment thread lib/primitiv.gi Outdated

Comment thread lib/primitiv.gi Outdated

Comment thread lib/primitiv.gi Outdated

fingolfin and others added 3 commits September 7, 2026 16:27

Merge branch 'master' into TB_NrMovedPoints_NrMovedPoints

4ba83a4

Both sides add tst/testinstall/interface.tst; keep the checks from either one, and declare `d` alongside `grps`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fingolfin approved these changes Sep 8, 2026

View reviewed changes

ThomasBreuer commented Sep 8, 2026

View reviewed changes

fingolfin and others added 2 commits September 8, 2026 23:18

fingolfin merged commit d46dbf1 into gap-packages:master Sep 9, 2026
4 of 5 checks passed

ThomasBreuer deleted the TB_NrMovedPoints_NrMovedPoints branch September 9, 2026 14:46

Uh oh!

Conversation

ThomasBreuer commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ThomasBreuer commented Sep 1, 2026

Uh oh!

fingolfin commented Sep 3, 2026

Uh oh!

ThomasBreuer commented Sep 3, 2026

Uh oh!

ThomasBreuer commented Sep 3, 2026

Uh oh!

fingolfin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fingolfin commented Sep 7, 2026

Uh oh!

fingolfin left a comment

Choose a reason for hiding this comment

Uh oh!

ThomasBreuer Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

ThomasBreuer Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ThomasBreuer commented Sep 1, 2026 •

edited

Loading

codecov Bot commented Sep 1, 2026 •

edited

Loading

ThomasBreuer Sep 8, 2026 •

edited

Loading