Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move permutation groups to new coercion model #24612

Closed
simon-king-jena opened this issue Jan 30, 2018 · 18 comments
Closed

Move permutation groups to new coercion model #24612

simon-king-jena opened this issue Jan 30, 2018 · 18 comments

Comments

@simon-king-jena
Copy link
Member

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

Component: coercion

Keywords: coercion subgroup, days94

Author: Travis Scrimshaw

Branch/Commit: 05ae254

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24612

@jdemeyer

This comment has been minimized.

@simon-king-jena
Copy link
Member Author

comment:2

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

@jdemeyer
Copy link

comment:3

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.

@jdemeyer jdemeyer changed the title Bug in coercion of subgroups into ambient group Move permutation groups to new coercion model Jun 12, 2018
@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

Commit: 344907e

@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

Author: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

New commits:

344907eMoving permutation groups to the new coercion model and style parents.

@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

Changed keywords from coercion subgroup to coercion subgroup, days94

@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

Changed commit from 344907e to 1b89e95

@tscrim
Copy link
Collaborator

tscrim commented Jun 30, 2018

New commits:

1b89e95Fixing doctests.

@jdemeyer
Copy link

jdemeyer commented Jul 3, 2018

comment:8

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.

@jdemeyer
Copy link

jdemeyer commented Jul 3, 2018

Reviewer: Jeroen Demeyer

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 3, 2018

Changed commit from 1b89e95 to 1815bc7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 3, 2018

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

1815bc7Minor fixes to permutation groups

@tscrim
Copy link
Collaborator

tscrim commented Jul 3, 2018

comment:10

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 3, 2018

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 3, 2018

Changed commit from 1815bc7 to 05ae254

@vbraun
Copy link
Member

vbraun commented Jul 7, 2018

Changed branch from public/coercion/perm_groups_new_parent-24612 to 05ae254

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants