Fix SimpleGroupsIterator for start values just below 10^18 - #6512
Merged
Conversation
The orders of the non-PSL(2,q) simple groups come in two lists, the second loaded on demand. The iterator asked for it once `start` reached 10^18, but the first list ends already at 911215823217986880, so for a start value between the two nothing was loaded, the search for the first relevant entry returned `fail`, and building the iterator broke on indexing the list with it. Load the second list when the search comes up empty instead, which is what `NextIterator_SimGp` already does, and report an order beyond the documented range rather than indexing with `fail` -- that case failed the same way above the second list. Assistance from Claude Code (Claude Opus 5): diagnosis from a stack trace, the fix, and the regression test. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
fingolfin
added
kind: bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6512 +/- ##
==========================================
+ Coverage 78.91% 78.94% +0.02%
==========================================
Files 685 685
Lines 294164 294222 +58
Branches 8669 8669
==========================================
+ Hits 232151 232266 +115
+ Misses 60210 60153 -57
Partials 1803 1803 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ThomasBreuer
approved these changes
Aug 16, 2026
ThomasBreuer
left a comment
ThomasBreuer
left a comment
Contributor
There was a problem hiding this comment.
Nice.
Once we are at it, the documentation can can be fixed as well.
| pos:=PositionProperty(SIMPLEGPSNONL2,x->x[1]>=start); | ||
| if pos=fail then | ||
| Error("List of simple groups only available up to order ", | ||
| SIMPLE_GROUPS_ITERATOR_RANGE); |
Contributor
There was a problem hiding this comment.
The effect is that already the SimpleGroupsIterator( start ) call errors with a useful message if start is too large, which is good. (This can be even tested cheaply now.)
The documentation of SimpleGroupsIterator
This function returns an iterator that will run over all nonabelian simple groups, starting at order start if specified, up to order 10^27 (or -- if specified -- order end).
is not correct if end is larger than 10^27.
| if start>=10^18 then LOADSIMPLE2(); fi; | ||
| pos:=First([1..Length(SIMPLEGPSNONL2)],x->SIMPLEGPSNONL2[x][1]>=start); | ||
| # Running off the end of the first list is what says the second one is | ||
| # needed. Testing `start' against a fixed bound instead left a window between |
Contributor
There was a problem hiding this comment.
This comment describes the current change.
As a comment in the code, it might be irritating.
Member
Author
There was a problem hiding this comment.
indeed. I've rewritten the comments
Say what the code does rather than what changed about it. Look the order up before searching for the PSL(2,q) order to start from, so that one beyond the range is rejected at once instead of after a search that takes minutes -- which is what makes it cheap to test. Shorten the message accordingly: with the 28-digit bound the old one wrapped, and matching that in a test needs a trailing space, which `Test` does not normalise away. The wording now matches what `lib/grp.gi` says for the same condition. The documentation claimed the iteration runs up to 10^27 or to `end`, which was wrong for an `end` beyond 10^27. Assistance from Claude Code (Claude Opus 5). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
The orders of the non-PSL(2,q) simple groups come in two lists, the second loaded on demand. The iterator asked for it once
startreached 10^18, but the first list ends already at 911215823217986880, so for a start value between the two nothing was loaded, the search for the first relevant entry returnedfail, and building the iterator broke on indexing the list with it.Load the second list when the search comes up empty instead, which is what
NextIterator_SimGpalready does, and report an order beyond the documented range rather than indexing withfail-- that case failed the same way above the second list.Assistance from Claude Code (Claude Opus 5): diagnosis from a stack trace, the fix, and the regression test.