-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PQuotient error for large groups. #5816
base: master
Are you sure you want to change the base?
Conversation
- Return fail in AbelianPQuotient if not enough generators for current parameters (logord in PQuotient, or #gens in collector of QuotientSystem qs) - Return fail or error in PQuotient for large groups, depending on option noninteractive - Update documentation of PQuotient - Add test for bugfix
|
||
# | ||
gap> PQuotient( FreeGroup(2), 5, 10, 520 : noninteractive ) <> fail; | ||
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually gives false
(hence CI fails)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is something I discuss right now with Hulpke in the original issue thread. The function does not behave like I would expect from the documentation. I already added this test case here, because in my opinion the function should behave like this. So in this case, the quotient can be generated by 520 elements, but the algorithm first needs to create a covering group that exceed this limit. Thus it currently throws fail. But actually I would expect it to not throw fail here.
## implementation produces an error message if the order of a | ||
## <M>p</M>-quotient exceeds <M>p^{256}</M> or <M>p^{<A>logord</A>}</M>, | ||
## respectively. | ||
## given. If the order of a <M>p</M>-quotient exceeds <M>p^{256}</M> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 256
an additional limit, or just the default value for logord
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is the default value, not an additional limit (at least what I can tell from the code and how it behaves on input groups whose quotients exceed 256 generators). Probably we should rephrase it. For now, I tried to stay as close to the original documentation as possible
Co-authored-by: Max Horn <max@quendi.de>
Closes #5809
Text for release notes
Fix PQuotient error for large groups: If
logord
is too small, we return fail or error depending on optionnoninteractive
.