Opened 4 years ago

Closed 4 years ago

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

Reported by: Owned by: soehms major sage-8.3 group theory days94 tscrim Sebastian Oehms Travis Scrimshaw N/A fb4481f #24612

### Description

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:
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:
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:
sage: img_mg = conv_ori(mg)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-2e064fe36e82> in <module>()
...............
ps/perm_gps/permgroup_element.c:4784)()
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]
259
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

### 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):
try:
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):
pass
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:

 ​0204811 `Added _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:

 ​344907e `Moving permutation groups to the new coercion model and style parents.` ​1b89e95 `Fixing doctests.` ​4d9dbdc `Merge branch 'public/coercion/perm_groups_new_parent-24612' of git://trac.sagemath.org/sage into permgrps_new_coercion` ​9ef1ea4 `Merge branch 'permgrps_new_coercion' into coerce_matrixgrps_to_permutationgrps` ​8f4d4d1 `merged 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:

 ​fb4481f `Merge 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.