-
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?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# see <https://github.com/gap-system/gap/issues/5809> | ||
gap> F := FreeGroup(["a", "b"]);; | ||
gap> a := F.1;; | ||
gap> b := F.1;; | ||
gap> p := 5;; | ||
gap> G := F / [a^p, b^p, Comm(a,b)];; | ||
gap> PQuotient(G, p, 1, 2 : noninteractive) <> fail; | ||
true | ||
gap> PQuotient(G, p, 1, 1 : noninteractive) = fail; | ||
true | ||
|
||
# | ||
gap> PQuotient( FreeGroup(2), 5, 10, 520 : noninteractive ) <> fail; | ||
true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually gives There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
gap> gPQuotient( FreeGroup(2), 5, 10, 519 : noninteractive ) = fail; | ||
FriedrichRober marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.
Is
256
an additional limit, or just the default value forlogord
?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