Opened 5 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: sage-8.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:

Status badges

Description (last modified by jdemeyer)

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

Change History (14)

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
  • Commit set to 344907ed5adc1ad687551d2f36d003707388ec34
  • Keywords days94 added
  • Milestone changed from sage-8.2 to sage-8.3
  • Status changed from new to needs_review

New commits:

344907eMoving 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

  • Commit changed from 344907ed5adc1ad687551d2f36d003707388ec34 to 1b89e952ae1ac35f13172bfbe29de4e15e868cc8
  • Status changed from needs_work to needs_review

New commits:

1b89e95Fixing 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:

1815bc7Minor 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:

5f39a40Merge branch 'public/coercion/perm_groups_new_parent-24612' of git://trac.sagemath.org/sage into public/coercion/perm_groups_new_parent-24612
05ae254Adding 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.