Fix SimpleGroupsIterator for start values just below 10^18 - #6512
Open
fingolfin wants to merge 1 commit into
Open
Fix SimpleGroupsIterator for start values just below 10^18#6512fingolfin wants to merge 1 commit into
SimpleGroupsIterator for start values just below 10^18#6512fingolfin wants to merge 1 commit into
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6512 +/- ##
=======================================
Coverage 78.91% 78.91%
=======================================
Files 685 685
Lines 294159 294170 +11
Branches 8669 8646 -23
=======================================
+ Hits 232142 232155 +13
+ Misses 60213 60212 -1
+ Partials 1804 1803 -1 ☔ 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.
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.