Use GroupHomset_libgap for permutation groups, as well

[soehms]

At the moment the construction of group homomorphisms between groups implemented under the classes ParentLibGAP and PermutationGroup_generic (and vice versa) is not possible in sage using the method hom. For example, it isn't possible to construct the natural projection from symplectic groups onto the corresponding projective group although this is possible using GAP:

sage: Sp43 = Sp(4,3)
sage: PSp43 = PSp(4,3)
sage: natProj = Sp43.hom(PSp43.gens())
Traceback (most recent call last):
...
TypeError: unable to convert [(3,4)(6,7)(9,10)(12,13)(17,20)(18,21)(19,22)(23,32)(24,33)(25,34)(26,38)(27,39)(28,40)(29,35)(30,36)(31,37), (1,5,14,17,27,22,19,36,3)(2,6,32)(4,7,23,20,37,13,16,26,40)(8,24,29,30,39,10,33,11,34)(9,15,35)(12,25,38)(21,28,31)] to an element of Set of Morphisms from Symplectic Group of degree 4 over Finite Field of size 3 to The projective symplectic linear group of degree 4 over Finite Field of size 3 in Category of finite groups

The reason for this is that the constructor of the class GroupHomset_libgap explicitly checks both groups to be instances of ParentLibGAP. So an easy way to have hom work in such cases, as well, would be to allow PermutationGroup_generic additionally.

An alternative option is to shift PermutationGroup_generic into the ParentLibGAP framework. But this would cause a lot of work to have all doctests pass through. The main problem here is that the element class of PermutationGroup_generic still has parent as second (optional) argument (in opposite to ParentLibGAP).

Therefore, this ticket will follow the first option!

[soehms]

I do the following:

1) I add the method gap to the classes PermutationGroup_generic and PermutationGroupElement to achieve compatibility with class GroupHomset_libgap and its elements class. For the same reason I add the method _subgroup_constructor to PermutationGroup_generic (needed in the methods pushforward, kernel and preimage in GroupMorphism_libgap)

2) I implement the method _Hom_ in PermutationGroup_generic to have hom use class GroupHomset_libgap.

3) In the module libgap_morphism I add PermutationGroup_generic to the list of allowed parent-instances at all relevant pieces of code. Furthermore, I changed the _element_constructor_ in order to avoid buffer size overflow by transferring long lists of large permutation generators to GAP. This is done in a new local method _obtain_gap_hom.

4) I take the occasion to implement two methods from framework: section of class Map and natural_map of class HomsetWithBase improving their default implementation using the possibilities of GroupHomset_libgap and its element class.

---

New commits:

​841a405	group morphism constructor from a libgap morphism
​08a0735	a doctest
​b2d9622	docs
​21cb664	correct docstring for argument `gap_hom`
​2a432d0	Merge commit '21cb66449cd424f94d15bd695fa986a0ce40c30c' of git://trac.sagemath.org/sage into libgap_morphism_permgroup-26750
​28f39df	initial version

[sbrandhorst]

I am not convinced the function _obtain_gap_hom is useful/necessary. It seems to be used only for the natural_map method. But then the code just belongs there. But indeed one could replace

gens = [g.gap() for g in dom.gens()]

by

gens = dom.gap().GeneratorsOfGroup()

in the _element_constructor_ and likewise for the images in natural map. Would that fix the overflow?

[git]

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

​deaa647

remove unnecessary method _obtain_gap_hom

[soehms]

Thanks for looking at this, Simon! Of course you are right: This method has been a stupid idea! I corrected as you suggested.

[git]

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

​0133335	implement subgroup / ambient methods for MatrixGroup_base
​747bc16	removed unused keyword
​c3a92a4	Merge branch 'develop' into matrix_groups_subgroups-25894
​901441b	Merge branch 'develop' into matrix_groups_subgroups-25894
​b02eb31	Merge branch 'libgap_morphism_permgroup-26750' into cubic_braid_group
​be4674b	initial implementation
​6a07854	Merge branch 'develop' into adapt_unitary_burau-26657
​806d0c6	Merge branch 'adapt_unitary_burau-26657' into cubic_braid_group
​464b784	Merge branch 'libgap_morphism_permgroup-26750' into cubic_braid_group
​2b74ce4	restore deleted doctest

[soehms]

New commits:

​0133335	implement subgroup / ambient methods for MatrixGroup_base
​747bc16	removed unused keyword
​c3a92a4	Merge branch 'develop' into matrix_groups_subgroups-25894
​901441b	Merge branch 'develop' into matrix_groups_subgroups-25894
​b02eb31	Merge branch 'libgap_morphism_permgroup-26750' into cubic_braid_group
​be4674b	initial implementation
​6a07854	Merge branch 'develop' into adapt_unitary_burau-26657
​806d0c6	Merge branch 'adapt_unitary_burau-26657' into cubic_braid_group
​464b784	Merge branch 'libgap_morphism_permgroup-26750' into cubic_braid_group
​2b74ce4	restore deleted doctest

[soehms]

New commits:

​0133335	implement subgroup / ambient methods for MatrixGroup_base
​747bc16	removed unused keyword
​c3a92a4	Merge branch 'develop' into matrix_groups_subgroups-25894
​901441b	Merge branch 'develop' into matrix_groups_subgroups-25894
​b02eb31	Merge branch 'libgap_morphism_permgroup-26750' into cubic_braid_group
​be4674b	initial implementation
​6a07854	Merge branch 'develop' into adapt_unitary_burau-26657
​806d0c6	Merge branch 'adapt_unitary_burau-26657' into cubic_braid_group
​464b784	Merge branch 'libgap_morphism_permgroup-26750' into cubic_braid_group
​2b74ce4	restore deleted doctest

[soehms]

I think I need some help. I wanted to restore the doctest of _obtain_gap_hom in _element_constructor_ but pushed from the wrong branch.

How can I go back to commit deaa64722319dc847871b77b15cab2efbb972850?

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​68582de

restore deleted doctest

[soehms]

I think I got it!

[sbrandhorst]

in the _element_constructor method:

if list(x) == codom.gens():
    # avoid buffer size overflow in the interface
    imgs = codom.gap().GeneratorsOfGroup()

this seems to fit better with the natural_map method? Put it there.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​a119816

revert changes in _element_constructor_ and improve method gap of permutation groups

[soehms]

Thanks for your useful hints, Simon! There where two problems:

1) I provided the wrong example, so that you did not have a chance to see what these lines of code should be good for. The correct example needs to have an instance of PermutationGroup_subgroup as codomain and be large enough to produce:

python2: libgap.c:184: libgap_get_input: Assertion `strlen(libGAP_stdin_buffer) < length' failed

2) The origin for this error, has not been in the _element_constructor_ method. So any changes here where useless, as you pointed out. Actually, the problem was that GAP didn't know about the subgroup construction.

So I revert all changes of code with respect to the _element_constructor_ and provide the example which caused the trouble in its docstring. I inserted a line of code in the constructor of the PermutationGroup_subgroup class to have the gap method of such a subgroup work well. This line I explain by a doctest in gap.

In addition I change the implementation of gap, since I realized that the former one can cause pickling errors.

[sbrandhorst]

It seems you are caching twice here. Once with the decorator once inside the method.

@cached_method
def gap(self):

---

New commits:

​e1e4da0

resetting to a new branch because of merge error

[soehms]

Replying to sbrandhorst:

It seems you are caching twice here. Once with the decorator once inside the method.

@cached_method
def gap(self):

---

New commits:

​e1e4da0

resetting to a new branch because of merge error

I inserted the decorator since I removed the explixit caching via the _libgap attribute (since this caused pickling problems). I am not aware that there is an implicite caching mechanism via the element construction of class Gap. Can you explain how this is working?

[dimpase]

I've linked this ticket to #26902, which aims at planning and tracking conversion of Sage library code from pexpect gap to libgap.

[dimpase]

Could you take care of pyflakes' advice (see patchbot output) and remove unneeded imports:

========== pyflakes ==========
git checkout patchbot/ticket_merged
src/sage/categories/homset.py:73: 'sage.misc.constant_function.ConstantFunction' imported but unused
src/sage/categories/homset.py:75: 'types' imported but unused
found 2 pyflakes errors in the modified files

[sbrandhorst]

Replying to soehms:

I inserted the decorator since I removed the explixit caching via the _libgap attribute (since this caused pickling problems). I am not aware that there is an implicite caching mechanism via the element construction of class Gap. Can you explain how this is working?

It still seems to be there:

+    @cached_method
+    def gap(self):
+        if self._libgap is not None:
+            return self._libgap
+        from sage.libs.gap.libgap import libgap
+        return libgap(self)

To me it looks like there are 2 caches once via the _libgap attribute and once in the @cached method Say if one first calls .gap() and then the _libgap attribute is set somewhere

[soehms]

Replying to sbrandhorst:

Replying to soehms: To me it looks like there are 2 caches once via the _libgap attribute and once in the @cached method Say if one first calls .gap() and then the _libgap attribute is set somewhere

In general it isn't cached via that attribute (I've removed that in my last change):

sage: S = SymmetricGroup(6)
sage: print S._libgap
None
sage: S.gap()
Sym( [ 1 .. 6 ] )
sage: print S._libgap
None

[git]

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

​70a87bf	Merge branch 'develop' into libgap_morphism_permgroup_26750
​4f4ec99	Merge branch 'develop' into libgap_morphism_permgroup_26750
​5b2c381	removed import statements according to pyflakes hints

[sbrandhorst]

+        from sage.libs.gap.element import GapElement_Permutation
+        if isinstance(img_gap, GapElement_Permutation):
+            return self.codomain()(img_gap.sage())

What happens here is a conversion of a gap element to an element of a permutation group. It would be more useful to let this be handled by the permuation group code i.e. in the element constructor of permutation groups and not in the _call_ method of the morphism. Also there are a few little python style remarks for the doctests, like

+            sage: GS=G.subgroup([G.gen(0)])

should be

+            sage: GS = G.subgroup([G.gen(0)])

[soehms]

I follow your suggestion concerning the _call_ method and I revert one of your style changes because of a failing doctest (see patchbot).

---

New commits:

​54a7f1b	Merge branch 'develop' into t/26750/libgap_morphism_permgroup_26750
​8bd5eff	cosmetic changes to the docs
​09b4696	move conversion to element construction of permutation

[sbrandhorst]

Positive review if you are happy with my changes & we have a green bot

---

New commits:

​e0feb00

changes to docs and the permutation group and subgroup constructor

[sbrandhorst]

btw. if you split up your tickets into smaller ones, they are easier to review

[soehms]

Replying to sbrandhorst:

Positive review if you are happy with my changes & we have a green bot

---

New commits:

​e0feb00

changes to docs and the permutation group and subgroup constructor

This is a good improvement in order to get the permutation groups closer to libgap! I just do two doc-string adaptions and rename your GapElement since that name is already used globally with respect to sage.interfaces.gap

Thanks for the review, Simon. I wish you a merry Christmas!

---

New commits:

​66ceb94

adapt doc-strings to former change and ticket #26889

[soehms]

Replying to sbrandhorst:

btw. if you split up your tickets into smaller ones, they are easier to review

Sure! I will try to take care of that the next time!

[soehms]

I've created a follow up ticket #27234 since by the improvements in connection with GAP 4.10 upgrade, the implementation of the gap method of class PermutationGroupElement can be done properly, right now.