Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25706 closed defect (fixed)

Conversion map from a matrix group to its corresponding permutation group does not work

Reported by: soehms Owned by:
Priority: major Milestone: sage-8.3
Component: group theory Keywords: days94
Cc: tscrim Merged in:
Authors: Sebastian Oehms Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: fb4481f (Commits, GitHub, GitLab) Commit:
Dependencies: #24612 Stopgaps:

Status badges


The problem is explained by the following example:

sage: G = GU(3,2); G
General Unitary Group of degree 3 over Finite Field in a of size 2^2
sage: MG = G.as_matrix_group(); MG
Matrix group over Finite Field in a of size 2^2 with 2 generators (
[a 0 0]  [a 1 1]
[0 1 0]  [1 1 0]
[0 0 a], [1 0 0]
sage: PG = MG.as_permutation_group(); PG
Permutation Group with generators [(2,3,5)(4,7,12)(6,10,18)(8,14,21)(9,16,22)(11,20,29)(13,23,33)(17,27,26)(19,28,34)(24,25,31)(30,36,35), (1,2,4,8,15,25,12,22,32,27,7,13)(3,6,11,21,31,36,18,16,26,20,30,33)(5,9,17,14,24,23)(10,19,29,28,35,34)]
sage: mg = MG.an_element(); mg
[a + 1     a     a]
[    1     1     0]
[    a     0     0]
sage: conv_ori = PG.convert_map_from(MG); conv_ori
Call morphism:
  From: Matrix group over Finite Field in a of size 2^2 with 2 generators (
[a 0 0]  [a 1 1]
[0 1 0]  [1 1 0]
[0 0 a], [1 0 0]
  To:   Permutation Group with generators [(2,3,5)(4,7,12)(6,10,18)(8,14,21)(9,16,22)(11,20,29)(13,23,33)(17,27,26)(19,28,34)(24,25,31)(30,36,35), (1,2,4,8,15,25,12,22,32,27,7,13)(3,6,11,21,31,36,18,16,26,20,30,33)(5,9,17,14,24,23)(10,19,29,28,35,34)]
sage: img_mg = conv_ori(mg)
TypeError                                 Traceback (most recent call last)
<ipython-input-8-2e064fe36e82> in <module>()
    256     else:
    257         if convert_dict is not None and needs_conversion:
--> 258             g = [tuple([convert_dict[x] for x in cycle])for cycle in g]
    260     return g

TypeError: 'sage.groups.matrix_gps.group_element.MatrixGroupElement_gap' object is not iterable

Remark: another (independent) problem is that the conversion can not be applied by instance call

Change History (15)

comment:1 Changed 4 years ago by tscrim

I would implement this as a coercion that could be discovered by the corresponding permutation group. So the pseudocode would be:

def _coerce_map_from_(self, G):
        PG = G.as_permutation_group()
        if PG == self:                                         
            return the_morphism_construced_from_GAP
        if self.has_coerce_map_from(PG):
            return self.coerce_map_from(PG) * the_morphism_to_PG
    except (AttributeError, TypeError):
    return super(PermutationGroup, self)._coerce_map_from_(G) 
Last edited 4 years ago by tscrim (previous) (diff)

comment:2 Changed 4 years ago by tscrim

However, this will likely not work until these become new-style parents: #24612.

comment:3 Changed 4 years ago by soehms

  • Branch set to u/soehms/coerce_matrixgrps_to_permutationgrps

comment:4 Changed 4 years ago by soehms

  • Commit set to 0204811fe8c63af581032f5f0c271be5d8a3ae97
  • Status changed from new to needs_review

New commits:

0204811Added _coerce_map_from_ to PermutationGroup_generic

comment:5 Changed 4 years ago by git

  • Commit changed from 0204811fe8c63af581032f5f0c271be5d8a3ae97 to 8f4d4d1689542c3d7bf9eb28a420ff39f8a616e0

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

344907eMoving permutation groups to the new coercion model and style parents.
1b89e95Fixing doctests.
4d9dbdcMerge branch 'public/coercion/perm_groups_new_parent-24612' of git:// into permgrps_new_coercion
9ef1ea4Merge branch 'permgrps_new_coercion' into coerce_matrixgrps_to_permutationgrps
8f4d4d1merged with #24612 and get it running again

comment:6 Changed 4 years ago by tscrim

  • Dependencies set to #24612
  • Reviewers set to Travis Scrimshaw

Positive review if a patchbot comes back green.

comment:7 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:8 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:9 Changed 4 years ago by tscrim

  • Branch changed from u/soehms/coerce_matrixgrps_to_permutationgrps to u/tscrim/matrix_gp_perm_gp_coercion-25706
  • Commit changed from 8f4d4d1689542c3d7bf9eb28a420ff39f8a616e0 to fb4481f2f2695b9e274025f6ae7a067c116c2ee4
  • Status changed from needs_work to positive_review

Trivial conflict.

New commits:

fb4481fMerge branch 'develop' into u/tscrim/matrix_gp_perm_gp_coercion-25706

comment:10 Changed 4 years ago by soehms

Hi Travis,

thanks for fixing this! Was it my fault? Did I have a chance to detect it by myself?

This week in Saragossa had been a very nice experience to me! I learned a lot (but need to learn much more, as these questions show) and met a lot of friendly people!

Thanks again, for making this possible to me!

comment:11 Changed 4 years ago by tscrim

No, it was my fault. They were my fault on the dependency and not thinking about conflicts.

It was great to meet you in Zaragoza; I hope you will be able to contribute more!

comment:12 Changed 4 years ago by vbraun

  • Branch changed from u/tscrim/matrix_gp_perm_gp_coercion-25706 to fb4481f2f2695b9e274025f6ae7a067c116c2ee4
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 4 years ago by dimpase

  • Commit fb4481f2f2695b9e274025f6ae7a067c116c2ee4 deleted

Hmm, why does this branch uses gap, not libgap?

Version 0, edited 4 years ago by dimpase (next)

comment:14 Changed 4 years ago by dimpase

No, in fact MatrixGroup is fine, it's PermutationGroup that needs work. Indeed, instead of that conversion to strings etc one can compute the libGAP group C in as_permutation_group() as follows:

sage: type(m)
<class 'sage.groups.matrix_gps.finitely_generated.FinitelyGeneratedMatrixGroup_gap_with_category'>
sage: iso=m._libgap_().IsomorphismPermGroup()
sage: C=iso.Image() # of the following, if you need:
sage: C=iso.Image().SmallerDegreePermutationRepresentation()
sage: PG=PermutationGroup(gap_group=gap(C.Image()),  canonicalize=False) # gap() is need, still

One way or another, it's much cleaner and faster, and easy to adapt to libgap-converted PermutationGroup. I've opened #26889 for this to be done.

comment:15 Changed 4 years ago by soehms

Hi Dima,

FYI: this has been one of my first tickets. I did it with the help of Travis on the SageDays94 in July in Zaragoza. At that time I didn't know much about libgap. In the meantime I learned a little bit more since I am trying to move the permutation groups closer to libgap (you aleady found the correspondig ticket: #26750).

In that context I had a look at as_permutation_group again (a couple of weeks ago), and I tried something similar as you did. I used the option algorithm in order not to change the default behavior (I wasn't sure if there is a proper reason for that construction which leads to a different result). But unfortunately, I realized that the new attribute _permutation_group_morphism (we had introduced in this ticket) does not keep track on optional arguments of as_permutation_group. This made me hesitating to continue that work.

But anyway, since the latter problem obviously is a bug, I opened a ticked about that, now: #26903

By the way: Do you know why the as_permutation_group of class FinitelyPresentedGroup does not use IsomorphismPermGroup?

Note: See TracTickets for help on using tickets.