Fix GAP interface problem in sylow_subgroup method

[simon-king-jena]

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.

Component: group theory

Keywords: GAP string representation

Reviewer: Simon King, Johan Sebastian Rosenkilde Nielsen, Mike Hansen

Issue created by migration from https://trac.sagemath.org/ticket/9621

[simon-king-jena]

Attachment: trac-9621_permgroup_sylow_subgroup.patch.gz

Fixes GAP interface bug of sylow_subgroup method (with doctest)

[johanrosenkilde]

comment:2

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.

[johanrosenkilde]

Simplification of the above patch

[simon-king-jena]

Attachment: trac_9621_simplification.patch.gz

Replaces the other two patches

[simon-king-jena]

Changed author from Simon King to Simon King, Johan Sebastian Rosenkilde Nielsen

[simon-king-jena]

comment:3

Attachment: trac-9621_permgroup_sylow_subgroup_with_simplification.patch.gz

Hi Johan!

Replying to @johanrosenkilde:

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

[johanrosenkilde]

comment:4

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 @simon-king-jena:

Hi Johan!

Replying to @johanrosenkilde:

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

[simon-king-jena]

comment:5

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...

[sagetrac-johanbosman]

Work Issues: rebase

[sagetrac-johanbosman]

comment:6

Replying to @simon-king-jena:

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. :).

[mwhansen]

comment:7

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

[simon-king-jena]

comment:8

Replying to @mwhansen:

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.

[simon-king-jena]

Reviewer: Mike Hansen

[jdemeyer]

Changed reviewer from Mike Hansen to Simon King, Johan Sebastian Rosenkilde Nielsen, Mike Hansen

[jdemeyer]

Changed author from Simon King, Johan Sebastian Rosenkilde Nielsen to none

[jdemeyer]

Changed work issues from rebase to none