Fix PQuotient error for large groups. #5816
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.
actually gives false (hence CI fails)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have put these lines out for now. I will open a new issue for this, since it is not clear if there is an easy fix for this.
#
gap> PQuotient( FreeGroup(2), 5, 10, 520 : noninteractive ) <> fail;
true
gap> PQuotient( FreeGroup(2), 5, 10, 519 : noninteractive ) = fail;
true
lib/pquot.gd
Outdated
| ## 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.
Is 256 an additional limit, or just the default value for logord ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I changed the documentation now
Co-authored-by: Max Horn <[email protected]>
|
@FriedrichRober last call for GAP 4.14 ... |
|
Hi, I will try to clean up in time, but can only start working on this on Monday. |
Attempt to fix confusion about logord parameter
|
@fingolfin @hulpke, I adjusted the documentation of the method. Since I am not an expert of this implementation, I would really appreciate if someone can proof-read this. As far as I understand, the current implementation throws an error/fail if the order of any p-group during the computation exceeds |
Closes #5809
Text for release notes
Fix PQuotient error for large groups: If
logordis too small, we return fail or error depending on optionnoninteractive.