Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#10334 closed enhancement (fixed)

miscellaneous cleanup in perm_gps preparing for domains

Reported by: Mike Hansen Owned by: joyner
Priority: minor Milestone: sage-4.7.1
Component: group theory Keywords: days30
Cc: Sage Combinat CC user, Jason B. Hill Merged in: sage-4.7.1.alpha1
Authors: Mike Hansen Reviewers: Robert Miller, Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (2)

trac_10334-permgroup_cleanup-mh.patch (66.9 KB) - added by Mike Hansen 12 years ago.
trac_10334-permgroup_cleanup-rebase-mh.patch (69.9 KB) - added by Nicolas M. Thiéry 11 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 12 years ago by Mike Hansen

Cc: Sage Combinat CC user Jason B. Hill added

comment:2 Changed 12 years ago by Mike Hansen

Status: newneeds_review

comment:3 Changed 12 years ago by Robert Miller

Reviewers: Robert Miller
Status: needs_reviewpositive_review

Looks good! Passes all long tests.

comment:4 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.2.alpha2
Resolution: fixed
Status: positive_reviewclosed

comment:5 Changed 12 years ago by François Bissey

I got a test failure that seems related to this patch

sage -t  "devel/sage-main/sage/groups/perm_gps/permgroup.py"
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 1876:
    sage: G.isomorphism_to(H)
Expected:
    Permutation group morphism:
      From: Permutation Group with generators [(2,3), (1,2,3)]
      To:   Permutation Group with generators [(1,2,4), (1,4)]
      Defn: [(2,3), (1,2,3)] -> [(2,4), (1,2,4)]
Got:
    Permutation group morphism:
      From: Permutation Group with generators [(2,3), (1,2,3)]
      To:   Permutation Group with generators [(1,2,4), (1,4)]
      Defn: [(2,3), (1,2,3)] -> [(2,4), (1,4,2)]

comment:6 Changed 12 years ago by Volker Braun

Status: closedneeds_work

I tripped over this patch as well (on sage-4.6.2.alpha3)

sage: S4 = SymmetricGroup(4)
sage: [ g.orbits() for g in S4.conjugacy_classes_subgroups() ]
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (396, 0))

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/home/vbraun/Sage/24cell/<ipython console> in <module>()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/misc/cachefunc.pyc in __call__(self, *args, **kwds)
    553             return self.cache[k]
    554         except KeyError:
--> 555             w = self._cachedmethod._instance_call(self._instance, *args, **kwds)
    556             self.cache[k] = w
    557             return w

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/misc/cachefunc.pyc in _instance_call(self, inst, *args, **kwds)
    776 
    777         """
--> 778         return self._cachedfunc.f(inst, *args, **kwds)
    779 
    780     def _get_instance_cache(self, inst):

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/groups/perm_gps/permgroup.pyc in orbits(self)
    952              sage: G.stabilizer(1) 
    953              Permutation Group with generators [(2,3)(4,10), (2,10,4)]
--> 954              sage: G = PermutationGroup([[(2,3,4)],[(6,7)]])
    955              sage: G.stabilizer(1)
    956              Permutation Group with generators [(6,7), (2,3,4)]

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/groups/perm_gps/permgroup.pyc in _set_gap(self)
    911          """
    912          return self._gap_().Orbit(integer).sage()
--> 913     
    914     def transversals(self, integer):
    915          """

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/groups/perm_gps/permgroup.pyc in set(self)
    896         return self._gap_().Orbits("[1..%d]" % self.largest_moved_point()).sage()
    897 
--> 898     @cached_method
    899     def orbit(self, integer):
    900          """

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:5600)()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2777)()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2663)()

AttributeError: 'PermutationGroup_generic_with_category' object has no attribute '_set'

For reference, I'll attach a patch that reverses the changes. With the patch reversed, I get the expected output.

comment:7 Changed 12 years ago by Mike Hansen

I've added a patch "trac_10334-permgroup_cleanup-fix-mh.patch" which fixes the issue that Volker found. The init method is cleaned up in #10335.

Let me know what the status of this ticket is.

comment:8 Changed 12 years ago by Volker Braun

Description: modified (diff)
Status: needs_workpositive_review

Works fine now! I can't reproduce François' doctest failure. He doesn't indicate if it is on a supported platform. We should go ahead and merge this one as it apparently works on the buildbot machines.

comment:9 Changed 12 years ago by François Bissey

Indeed I didn't. Turns out it is some crap I have in gap. I'll open another ticket if I find anything that should be fixed there. Nothing relevant to this ticket, it just pointed to the failure of another component.

comment:10 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.2.alpha2

What's up with this ticket? It was merged in sage-4.6.2.alpha2 and worked fine on the buildbot.

I'm going to unmerge this in sage-4.6.2.alpha4 until this is sorted out.

Volker: it is a very bad idea to reopen tickets which are already merged, this will cause major confusion. At the very least, it should be discussed with the release manager (me in this case).

comment:11 Changed 12 years ago by Mike Hansen

It just needs trac_10334-permgroup_cleanup-mh.patch and trac_10334-permgroup_cleanup-fix-mh.patch to be applied. The first was already merged in alpha2.

comment:12 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)

comment:13 Changed 12 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

Please change the commit messages of the patches to something more descriptive (and keep in mind that the ticket number should be on the first line of the commit message).

Changed 12 years ago by Mike Hansen

comment:14 Changed 12 years ago by Mike Hansen

Description: modified (diff)
Status: needs_workpositive_review

I've fixed the commit message and combined the two patches into one.

comment:15 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.2.rc0
Status: positive_reviewclosed

comment:16 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.2.rc0
Milestone: sage-4.6.2sage-4.7
Priority: majorminor
Resolution: fixed
Status: closednew

Doctest error on 64-bit OS X 10.6:

sage -t -long  -force_lib devel/sage/sage/groups/perm_gps/permgroup.py
**********************************************************************
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.6.2.rc0/devel/sage-main/sage/groups/perm_gps/permgroup.py", line 1174:
    sage: G.random_element()
Expected:
    (1,2)(4,5)
Got:
    (2,3)(4,5)
**********************************************************************

comment:17 Changed 12 years ago by Jeroen Demeyer

Status: newneeds_work

comment:18 Changed 12 years ago by Rob Beezer

FWIW, I applied this to 4.7.alpha1 with a single (easy-to-fix) hunk failure. I could post a rebase if that is desirable.

Passed all tests in sage/groups, and in particular I did not see the "random element" failure noted above (64-bit Ubuntu).

comment:19 Changed 11 years ago by Rob Beezer

Description: modified (diff)

On 4.7.rc0.

1) 3 hunk failures when applying trac_10334-permgroup_cleanup-rebase-mh.patch, a rebase is attached.

2) With rebase a quotient group fails in a doctest. Here's the evidence:

Clean 4.7.rc0:

G = DihedralGroup(20)
C = G.commutator()
Q = G.quotient(C)
Q.is_abelian()

4.7.rc0 with rebased patch:

sage: G = DihedralGroup(20)
sage: C = G.commutator()
sage: Q = G.quotient(C)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/sage/sage-4.7.rc0/devel/sage-main/<ipython console> in <module>()

/sage/sage-4.7.rc0/local/lib/python2.6/site-packages/sage/groups/perm_gps/permgroup.pyc in quotient(self, N)
   1483         phi = Q.RegularActionHomomorphism()
   1484         gens = phi.Image().GeneratorsOfGroup()
-> 1485         return PermutationGroup([self(gen) for gen in gens])
   1486
   1487     def quotient_group(self, N):

/sage/sage-4.7.rc0/local/lib/python2.6/site-packages/sage/groups/perm_gps/permgroup.pyc in __call__(self, x, check)
    593             return self.identity()
    594
--> 595         return self._element_class()(x, self, check=check)
    596
    597     def _coerce_impl(self, x):

/sage/sage-4.7.rc0/local/lib/python2.6/site-packages/sage/groups/perm_gps/permgroup_element.so in sage.groups.perm_gps.permgroup_element.PermutationGroupElement.__init__ (sage/groups/perm_gps/permgroup_element.c:3008)()

TypeError: permutation (1, 2)(3, 4) not in Dihedral group of order 40 as a permutation group

I've tried to locate the error, with no luck. My rebase could be at fault, but I've double-checked that as well.

comment:20 Changed 11 years ago by Rob Beezer

Forgot some details above.

On clean 4.7.rc0 the "is_abelian" returns {{{True}}. This is a doctest that has been passing in alpha testing.

On patched version, the commutator is a subgroup, and it is normal:

sage: G = DihedralGroup(20)
sage: C = G.commutator()
sage: C.is_subgroup(G)
True
sage: C.is_normal(G)
True

Changed 11 years ago by Nicolas M. Thiéry

comment:21 Changed 11 years ago by Nicolas M. Thiéry

Reviewers: Robert MillerRobert Miller, Robert Beezer
Status: needs_workneeds_review

Fixed a remaining doctest with Rob.

comment:22 Changed 11 years ago by Rob Beezer

For the record, it looks like quotients were failing because elements of the permutation group representation of the quotient were being coerced into the larger of the two groups used in the quotient construction. Since the representation of the quotient has little connection with this larger group the changes to the __init__ method would now notice and fail.

comment:23 Changed 11 years ago by Rob Beezer

Milestone: sage-4.7sage-4.7.1
Status: needs_reviewpositive_review

Passes all tests on 4.7.rc0 and looks good.

Lots of good improvements and fixes in here. Thanks, Mike.

comment:24 Changed 11 years ago by Nicolas M. Thiéry

Keywords: days30 added

comment:25 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.7.1.alpha1
Resolution: fixed
Status: positive_reviewclosed

comment:26 Changed 11 years ago by Jeroen Demeyer

Reviewers: Robert Miller, Robert BeezerRobert Miller, Rob Beezer

comment:27 Changed 11 years ago by Mike Hansen

Test comment.

comment:28 Changed 11 years ago by Mike Hansen

And another test comment.

Note: See TracTickets for help on using tickets.