Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#1155 closed defect (fixed)

[with patch, with positive review] PermutationGroup coercion bug

Reported by: wdj Owned by: mhansen
Priority: minor Milestone: sage-2.10.1
Component: combinatorics Keywords: GAP, permutation group
Cc: sage-combinat Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The patch http://sage.math.washington.edu/home/wdj/patches/permgp-2007-11-12.hg fixes a bug reported by Carlo H. Part of his email is pasted below:

+++++++++++++++++++++++++++++++++++++++++++++++

Hi,

I'm doing some work with groups. Using gap.Image() I can get a permutation like this:

sage: x
(1,2)(3,7)(4,6)(5,8)

But to make a permutation group out of this element I have to enclose the x in two sets of brackets:

sage: PermutationGroup([[x]])
Permutation Group with generators [(1,2)(3,7)(4,6)(5,8)]

On the other hand, the following command fails (see below for code and output):

sage: PermutationGroup([x])

In my mind the second version is clearer - x is a permutation so [x] is a list of permutations and I should be able to use that to get a group.

Should SAGE do a coercion here, or am I doing something in a strange way?

Code and output:

----------------------------------------------------------------------
| SAGE Version 2.8.11, Release Date: 2007-11-02                      |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------

sage: p = 2
sage: assert is_prime(p)
sage:
sage: F = gap.new("FreeGroup(3)")
sage:
sage: a = F.gen(1)
sage: b = F.gen(2)
sage: c = F.gen(3)
sage:
sage: rels = []
sage: rels.append( a**Integer(p) )
sage: rels.append( b**Integer(p) )
sage: rels.append( c**Integer(p) )
sage: rels.append( a*b*((b*a*c)**Integer(-1)) )
sage: rels.append( c*a*((a*c)**Integer(-1)) )
sage: rels.append( c*b*((b*c)**Integer(-1)) )
sage:
sage: N = gap.NormalClosure(F, gap.Subgroup(F, rels))
sage: niso = gap.NaturalHomomorphismByNormalSubgroupNC(F, N)
sage:
sage: x = gap.Image(niso, a)
sage: x
(1,2)(3,7)(4,6)(5,8)
sage: PermutationGroup([[x]])
Permutation Group with generators [(1,2)(3,7)(4,6)(5,8)]
sage:
sage: PermutationGroup([x])
---------------------------------------------------------------------------
<type 'exceptions.TypeError'>             Traceback (most recent call last)

Attachments (1)

1155.patch (1.6 KB) - added by mhansen 12 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by mabshoff

  • Milestone changed from sage-2.9 to sage-2.8.13
  • Summary changed from PermutationGroup coercion bug to [with bundle] PermutationGroup coercion bug

comment:2 Changed 12 years ago by cwitty

  • Summary changed from [with bundle] PermutationGroup coercion bug to [with bundle, with negative review] PermutationGroup coercion bug

This patch doesn't feel right to me; it seems like it's fixing the problem at the wrong level. (For instance, it sometimes breaks if you try to create a permutation group from a list of generators where some of the generators are Python lists and some are Gap permutation group elements. Maybe that's too strange a case to worry about?)

I haven't tried it, but it looks like adding a special case to gap_format() for Gap permutation group elements would also fix the problem, perhaps in a better way?

comment:3 Changed 12 years ago by cwitty

Also, there are no doctests in the patch.

comment:4 Changed 12 years ago by mhansen

  • Owner changed from wdj to mhansen
  • Status changed from new to assigned
  • Summary changed from [with bundle, with negative review] PermutationGroup coercion bug to [with patch] PermutationGroup coercion bug

I've put a new patch up.

Changed 12 years ago by mhansen

comment:5 Changed 12 years ago by rlm

  • Summary changed from [with patch] PermutationGroup coercion bug to [with patch, needs review] PermutationGroup coercion bug

comment:6 Changed 12 years ago by cwitty

  • Summary changed from [with patch, needs review] PermutationGroup coercion bug to [with patch, with positive review] PermutationGroup coercion bug

Code looks good; doctest passes.

comment:7 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in Sage 2.10.1.rc1

comment:8 Changed 11 years ago by nthiery

  • Cc sage-combinat added
Note: See TracTickets for help on using tickets.