Opened 5 years ago

Closed 4 years ago

# Move permutation groups to new coercion model

Reported by: Owned by: SimonKing critical sage-8.3 coercion coercion subgroup, days94 Travis Scrimshaw Jeroen Demeyer N/A 05ae254 05ae2540ab498b19c26832f2f8c529c75155b6b9

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)
<ipython-input-13-9acf54a006a7> in <module>()
----> 1 f(S4[Integer(1)])

/usr/local/src/sage-config/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/sage-config/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/sage-config/local/lib/python2.7/site-packages/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/sage-config/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)]
```

### comment:1 Changed 5 years ago by jdemeyer

• Description modified (diff)

### comment:2 Changed 5 years ago by SimonKing

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).

### comment:3 Changed 4 years ago by jdemeyer

Permutation groups still use the old coercion model. So moving to the new coercion model (unfortunately a non-trivial operation) is almost certainly going to fix this.

### comment:4 Changed 4 years ago by jdemeyer

• 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 tscrim

• Authors set to Travis Scrimshaw
• Branch set to public/coercion/perm_groups_new_parent-24612
• Milestone changed from sage-8.2 to sage-8.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 jdemeyer

• Status changed from needs_review to needs_work

### comment:7 Changed 4 years ago by tscrim

• Status changed from needs_work to needs_review

New commits:

 ​1b89e95 `Fixing doctests.`

### comment:8 Changed 4 years ago by jdemeyer

• 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 git

• 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 tscrim

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 git

• Commit changed from 1815bc742792da2a96577af6a044aa814f912bf8 to 05ae2540ab498b19c26832f2f8c529c75155b6b9

Branch pushed to git repo; I updated commit sha1. New commits:

 ​5f39a40 `Merge branch 'public/coercion/perm_groups_new_parent-24612' of git://trac.sagemath.org/sage into public/coercion/perm_groups_new_parent-24612` ​05ae254 `Adding comment in _element_constructor_.`

### comment:12 Changed 4 years ago by tscrim

• Status changed from needs_info to needs_review

### comment:13 Changed 4 years ago by jdemeyer

• Status changed from needs_review to positive_review

### comment:14 Changed 4 years ago by vbraun

• Branch changed from public/coercion/perm_groups_new_parent-24612 to 05ae2540ab498b19c26832f2f8c529c75155b6b9
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.