Opened 11 years ago
Closed 9 years ago
#9621 closed defect (duplicate)
Fix GAP interface problem in sylow_subgroup method
Reported by: | SimonKing | Owned by: | joyner |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | group theory | Keywords: | GAP string representation |
Cc: | Merged in: | ||
Authors: | Reviewers: | Simon King, Johan Sebastian Rosenkilde Nielsen, Mike Hansen | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
The following was reported by Kenny Brown:
sage: n = 3^2 * 7^2 sage: G = CyclicPermutationGroup(n) sage: G.sylow_subgroup(3) Traceback (most recent call last): ...
The problem is that in the sylow_subgroup method, it is attempted to get the string presentation of a permutation in GAP by calling gap.eval(...). However, GAP truncates the output. So, better use gap.eval('Print(...)') instead.
Moreover, the method uses quite generic variable names in the GAP interface. This is dangerous, as the use of variable names that any average user might choose as well can have nasty side effects.
Fixed by #10334.
Attachments (3)
Change History (12)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 11 years ago by
Bear with me as this is my first action on the ticket system :-)
It seems that some parsing functionality has already been built into the gap interface, so all the last lines of sylow_subgroups can be greatly simplified. I have added an extra patch file for this.
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 11 years ago by
Hi Johan!
Replying to jsrn:
It seems that some parsing functionality has already been built into the gap interface, so all the last lines of sylow_subgroups can be greatly simplified.
You have a misprint in your patch. You wrote self_element_class(), but it should be self._element_class().
However, your suggestion makes indeed sense. So, I created a patch that corrects that misprint and combines both of our patches into one.
Now the big question is: I think we are both Authors now (and I inserted your name in the corresponding field of this ticket). So, who will review??
Cheers, Simon
comment:4 in reply to: ↑ 3 Changed 11 years ago by
Ah, embarrassing; I had seen that mistake, but must have forgotten to remake the patch or something :-)
Thanks for adding me as author. I guess we'll have to wait for a nice person to come along and review the (final?) patch.
Regards, Johan
Replying to SimonKing:
Hi Johan!
Replying to jsrn:
It seems that some parsing functionality has already been built into the gap interface, so all the last lines of sylow_subgroups can be greatly simplified.
You have a misprint in your patch. You wrote self_element_class(), but it should be self._element_class().
However, your suggestion makes indeed sense. So, I created a patch that corrects that misprint and combines both of our patches into one.
Now the big question is: I think we are both Authors now (and I inserted your name in the corresponding field of this ticket). So, who will review??
Cheers, Simon
comment:5 follow-up: ↓ 6 Changed 10 years ago by
Apply trac-9621_permgroup_sylow_subgroup_with_simplification.patch
(For the patchbot)
Probably this patch is bit rotting and we need rebasing. But who knows, perhaps we are lucky...
comment:6 in reply to: ↑ 5 Changed 9 years ago by
- Status changed from needs_review to needs_work
- Work issues set to rebase
Replying to SimonKing:
Apply trac-9621_permgroup_sylow_subgroup_with_simplification.patch
(For the patchbot)
Probably this patch is bit rotting and we need rebasing.
Indeed we do. :).
comment:7 follow-up: ↓ 8 Changed 9 years ago by
I think this can be closed as I fixed this problem in #10334.
comment:8 in reply to: ↑ 7 Changed 9 years ago by
- Milestone changed from sage-5.0 to sage-duplicate/invalid/wontfix
- Reviewers set to Mike Hansen
- Status changed from needs_work to positive_review
comment:9 Changed 9 years ago by
- Description modified (diff)
- Resolution set to duplicate
- Reviewers changed from Mike Hansen to Simon King, Johan Sebastian Rosenkilde Nielsen, Mike Hansen
- Status changed from positive_review to closed
- Work issues rebase deleted
Fixes GAP interface bug of sylow_subgroup method (with doctest)