Opened 4 years ago
Closed 4 years ago
#24612 closed defect (fixed)
Move permutation groups to new coercion model
Reported by:  SimonKing  Owned by:  

Priority:  critical  Milestone:  sage8.3 
Component:  coercion  Keywords:  coercion subgroup, days94 
Cc:  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  05ae254 (Commits, GitHub, GitLab)  Commit:  05ae2540ab498b19c26832f2f8c529c75155b6b9 
Dependencies:  Stopgaps: 
Description (last modified by )
We have
sage: S4 = SymmetricGroup(4) sage: G = S4.subgroups()[19] sage: f = G.coerce_map_from(S4); f Call morphism: From: Symmetric group of order 4! as a permutation group To: Subgroup of (Symmetric group of order 4! as a permutation group) generated by [(1,3)(2,4), (1,4,3,2)]
This is very wrong, there is no coercion from S4
to G
:
sage: f(S4[1])  TypeError Traceback (most recent call last) <ipythoninput139acf54a006a7> in <module>() > 1 f(S4[Integer(1)]) /usr/local/src/sageconfig/src/sage/categories/map.pyx in sage.categories.map.Map.__call__ (build/cythonized/sage/categories/map.c:6632)() 769 if P is D: # we certainly want to call _call_/with_args 770 if not args and not kwds: > 771 return self._call_(x) 772 return self._call_with_args(x, args, kwds) 773 # Is there coercion? /usr/local/src/sageconfig/src/sage/categories/morphism.pyx in sage.categories.morphism.CallMorphism._call_ (build/cythonized/sage/categories/morphism.c:6790)() 420 421 cpdef Element _call_(self, x): > 422 return self._codomain(x) 423 424 cdef class IdentityMorphism(Morphism): /usr/local/src/sageconfig/local/lib/python2.7/sitepackages/sage/groups/perm_gps/permgroup.pyc in __call__(self, x, check) 678 return self.identity() 679 > 680 return self._element_class()(x, self, check=check) 681 682 def _coerce_impl(self, x): /usr/local/src/sageconfig/src/sage/groups/perm_gps/permgroup_element.pyx in sage.groups.perm_gps.permgroup_element.PermutationGroupElement.__init__ (build/cythonized/sage/groups/perm_gps/permgroup_element.c:5749)() 462 P = parent._gap_() 463 if not P.parent()(self.__gap) in P: > 464 raise TypeError('permutation %s not in %s' % (g, parent)) 465 466 Element.__init__(self, parent) TypeError: permutation (1,2) not in Subgroup of (Symmetric group of order 4! as a permutation group) generated by [(1,3)(2,4), (1,4,3,2)]
Change History (14)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
comment:3 Changed 4 years ago by
Permutation groups still use the old coercion model. So moving to the new coercion model (unfortunately a nontrivial operation) is almost certainly going to fix this.
comment:4 Changed 4 years ago by
 Summary changed from Bug in coercion of subgroups into ambient group to Move permutation groups to new coercion model
comment:5 Changed 4 years ago by
 Branch set to public/coercion/perm_groups_new_parent24612
 Commit set to 344907ed5adc1ad687551d2f36d003707388ec34
 Keywords days94 added
 Milestone changed from sage8.2 to sage8.3
 Status changed from new to needs_review
New commits:
344907e  Moving permutation groups to the new coercion model and style parents.

comment:6 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:7 Changed 4 years ago by
 Commit changed from 344907ed5adc1ad687551d2f36d003707388ec34 to 1b89e952ae1ac35f13172bfbe29de4e15e868cc8
 Status changed from needs_work to needs_review
New commits:
1b89e95  Fixing doctests.

comment:8 Changed 4 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to needs_info
Why this in _element_constructor_
?
if isinstance(x, PermutationGroupElement): x_parent = x.parent() if (isinstance(x_parent, PermutationGroup_subgroup) and x_parent._ambient_group is self): return self.element_class(x.cycle_tuples(), self, check=False) from sage.groups.perm_gps.permgroup_named import SymmetricGroup compatible_domains = all(point in self._domain_to_gap for point in x_parent.domain()) if compatible_domains and (isinstance(self, SymmetricGroup) or x._gap_() in self._gap_()): return self.element_class(x.cycle_tuples(), self, check=False)
All tests in src/sage/groups
pass when removing that block of code.
comment:9 Changed 4 years ago by
 Commit changed from 1b89e952ae1ac35f13172bfbe29de4e15e868cc8 to 1815bc742792da2a96577af6a044aa814f912bf8
Branch pushed to git repo; I updated commit sha1. New commits:
1815bc7  Minor fixes to permutation groups

comment:10 Changed 4 years ago by
That is to support the old "coercions" as conversions that were previously supported. I don't think such a thing was explicitly tested, but I wanted to mitigate the number of behavioral changes.
comment:11 Changed 4 years ago by
 Commit changed from 1815bc742792da2a96577af6a044aa814f912bf8 to 05ae2540ab498b19c26832f2f8c529c75155b6b9
comment:12 Changed 4 years ago by
 Status changed from needs_info to needs_review
comment:13 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:14 Changed 4 years ago by
 Branch changed from public/coercion/perm_groups_new_parent24612 to 05ae2540ab498b19c26832f2f8c529c75155b6b9
 Resolution set to fixed
 Status changed from positive_review to closed
Aha! So, the problem is that Sage thinks there is a coercion from the group to the subgroup, and not only from the subgroup to the group? That would indeed explain the current behaviour (multiplying a group element with a subgroup element works, multiplying a subgroup element by a group element doesn't work).