#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: |
Description (last modified by )
Attachments (2)
Change History (30)
comment:1 Changed 12 years ago by
Cc: | Sage Combinat CC user Jason B. Hill added |
---|
comment:2 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 12 years ago by
Reviewers: | → Robert Miller |
---|---|
Status: | needs_review → positive_review |
comment:4 Changed 12 years ago by
Merged in: | → sage-4.6.2.alpha2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:5 Changed 12 years ago by
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
Status: | closed → 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 12 years ago by
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
Description: | modified (diff) |
---|---|
Status: | needs_work → 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 12 years ago by
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
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
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
Description: | modified (diff) |
---|
comment:13 Changed 12 years ago by
Status: | positive_review → 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 12 years ago by
Attachment: | trac_10334-permgroup_cleanup-mh.patch added |
---|
comment:14 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → positive_review |
I've fixed the commit message and combined the two patches into one.
comment:15 Changed 12 years ago by
Merged in: | → sage-4.6.2.rc0 |
---|---|
Status: | positive_review → closed |
comment:16 Changed 12 years ago by
Merged in: | sage-4.6.2.rc0 |
---|---|
Milestone: | sage-4.6.2 → sage-4.7 |
Priority: | major → minor |
Resolution: | fixed |
Status: | closed → 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 12 years ago by
Status: | new → needs_work |
---|
comment:18 Changed 12 years ago by
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
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
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
Attachment: | trac_10334-permgroup_cleanup-rebase-mh.patch added |
---|
comment:21 Changed 11 years ago by
Reviewers: | Robert Miller → Robert Miller, Robert Beezer |
---|---|
Status: | needs_work → needs_review |
Fixed a remaining doctest with Rob.
comment:22 Changed 11 years ago by
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
Milestone: | sage-4.7 → sage-4.7.1 |
---|---|
Status: | needs_review → 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 11 years ago by
Keywords: | days30 added |
---|
comment:25 Changed 11 years ago by
Merged in: | → sage-4.7.1.alpha1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:26 Changed 11 years ago by
Reviewers: | Robert Miller, Robert Beezer → Robert Miller, Rob Beezer |
---|
Looks good! Passes all long tests.