Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#10334 closed enhancement (fixed)

miscellaneous cleanup in perm_gps preparing for domains

Reported by: mhansen Owned by: joyner
Priority: minor Milestone: sage-4.7.1
Component: group theory Keywords: days30
Cc: sage-combinat, jasonbhill 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:

Attachments (2)

trac_10334-permgroup_cleanup-mh.patch (66.9 KB) - added by mhansen 9 years ago.
trac_10334-permgroup_cleanup-rebase-mh.patch (69.9 KB) - added by nthiery 8 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by mhansen

  • Cc sage-combinat jasonbhill added

comment:2 Changed 9 years ago by mhansen

  • Status changed from new to needs_review

comment:3 Changed 9 years ago by rlm

  • Reviewers set to Robert Miller
  • Status changed from needs_review to positive_review

Looks good! Passes all long tests.

comment:4 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:5 Changed 9 years ago by fbissey

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 9 years ago by vbraun

  • Status changed from closed to needs_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 9 years ago by mhansen

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 9 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_work to positive_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 9 years ago by fbissey

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 9 years ago by jdemeyer

  • Merged in sage-4.6.2.alpha2 deleted

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

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 9 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_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 9 years ago by mhansen

comment:14 Changed 9 years ago by mhansen

  • Description modified (diff)
  • Status changed from needs_work to positive_review

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

comment:15 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.2.rc0
  • Status changed from positive_review to closed

comment:16 Changed 9 years ago by jdemeyer

  • Merged in sage-4.6.2.rc0 deleted
  • Milestone changed from sage-4.6.2 to sage-4.7
  • Priority changed from major to minor
  • Resolution fixed deleted
  • Status changed from closed to new

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 9 years ago by jdemeyer

  • Status changed from new to needs_work

comment:18 Changed 9 years ago by rbeezer

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 8 years ago by rbeezer

  • 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 8 years ago by rbeezer

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 8 years ago by nthiery

comment:21 Changed 8 years ago by nthiery

  • Reviewers changed from Robert Miller to Robert Miller, Robert Beezer
  • Status changed from needs_work to needs_review

Fixed a remaining doctest with Rob.

comment:22 Changed 8 years ago by rbeezer

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 8 years ago by rbeezer

  • Milestone changed from sage-4.7 to sage-4.7.1
  • Status changed from needs_review to positive_review

Passes all tests on 4.7.rc0 and looks good.

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

comment:24 Changed 8 years ago by nthiery

  • Keywords days30 added

comment:25 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:26 Changed 8 years ago by jdemeyer

  • Reviewers changed from Robert Miller, Robert Beezer to Robert Miller, Rob Beezer

comment:27 Changed 8 years ago by mhansen

Test comment.

comment:28 Changed 8 years ago by mhansen

And another test comment.

Note: See TracTickets for help on using tickets.