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:

Status badges

Description (last modified by jdemeyer)

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)

trac-9621_permgroup_sylow_subgroup.patch (5.1 KB) - added by SimonKing 11 years ago.
Fixes GAP interface bug of sylow_subgroup method (with doctest)
trac_9621_simplification.patch (1007 bytes) - added by jsrn 11 years ago.
Simplification of the above patch
trac-9621_permgroup_sylow_subgroup_with_simplification.patch (5.1 KB) - added by SimonKing 11 years ago.
Replaces the other two patches

Download all attachments as: .zip

Change History (12)

Changed 11 years ago by SimonKing

Fixes GAP interface bug of sylow_subgroup method (with doctest)

comment:1 Changed 11 years ago by SimonKing

  • Status changed from new to needs_review

comment:2 follow-up: Changed 11 years ago by jsrn

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.

Changed 11 years ago by jsrn

Simplification of the above patch

Changed 11 years ago by SimonKing

Replaces the other two patches

comment:3 in reply to: ↑ 2 ; follow-up: Changed 11 years ago by SimonKing

  • Authors changed from Simon King to Simon King, Johan Sebastian Rosenkilde Nielsen

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 jsrn

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: Changed 10 years ago by SimonKing

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 johanbosman

  • 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: Changed 9 years ago by mhansen

I think this can be closed as I fixed this problem in #10334.

comment:8 in reply to: ↑ 7 Changed 9 years ago by SimonKing

  • Milestone changed from sage-5.0 to sage-duplicate/invalid/wontfix
  • Reviewers set to Mike Hansen
  • Status changed from needs_work to positive_review

Replying to mhansen:

I think this can be closed as I fixed this problem in #10334.

OK, I just tested that it works with sage-5.0.rc0. Hence, it is a duplicate (or sub-problem) of #10334.

comment:9 Changed 9 years ago by jdemeyer

  • Authors Simon King, Johan Sebastian Rosenkilde Nielsen deleted
  • 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
Note: See TracTickets for help on using tickets.