#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: |
Description (last modified by )
Attachments (2)
Change History (30)
comment:1 Changed 8 years ago by
- Cc sage-combinat jasonbhill added
comment:2 Changed 8 years ago by
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- Reviewers set to Robert Miller
- Status changed from needs_review to positive_review
comment:4 Changed 8 years ago by
- Merged in set to sage-4.6.2.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:5 Changed 8 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 8 years ago by
- 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 8 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 8 years ago by
- 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 8 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 8 years ago by
- 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 8 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 8 years ago by
- Description modified (diff)
comment:13 Changed 8 years ago by
- 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 8 years ago by
comment:14 Changed 8 years ago by
- 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 8 years ago by
- Merged in set to sage-4.6.2.rc0
- Status changed from positive_review to closed
comment:16 Changed 8 years ago by
- 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 8 years ago by
- Status changed from new to needs_work
comment:18 Changed 8 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 8 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 8 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 8 years ago by
comment:21 Changed 8 years ago by
- 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
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
- 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
- Keywords days30 added
comment:25 Changed 8 years ago by
- 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
- Reviewers changed from Robert Miller, Robert Beezer to Robert Miller, Rob Beezer
comment:27 Changed 8 years ago by
Test comment.
comment:28 Changed 8 years ago by
And another test comment.
Looks good! Passes all long tests.