do not cache gap twice

[mantepse]

Currently, in permgroup.py, we are apparently caching the gap function twice: once using the decorator, and once by putting the result into an attribute.

Apparently, removing the decorator is a performance improvement:

sage: from sage.rings.species import MolecularSpecies, PolynomialSpecies
sage: M = MolecularSpecies("X"); P = PolynomialSpecies(QQ, "X")
sage: n=5; l = [P(m.permutation_group()[0]) for m in M.subset(n)]

sage: %timeit [p.hadamard_product(q) for p in l for q in l]

gives me

6.98 s ± 189 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

with the decorator and

sage: %timeit [p.hadamard_product(q) for p in l for q in l]
6.35 s ± 198 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

without.

What I find very surprising is that the following is much slower. I would really like to know why.

diff --git a/src/sage/groups/perm_gps/permgroup.py b/src/sage/groups/perm_gps/permgroup.py
index cf8700476f8..df224194d2c 100644
--- a/src/sage/groups/perm_gps/permgroup.py
+++ b/src/sage/groups/perm_gps/permgroup.py
@@ -697,7 +697,8 @@ class PermutationGroup_generic(FiniteGroup):
         if self._libgap is None:
             # why is self._libgap = libgap.Group(self._gens) and
             # adding methods to permgroup_named slower?
-            self._libgap = super()._libgap_()
+            # self._libgap = super()._libgap_()
+            self._libgap = libgap.Group(self._gens)
         return self._libgap
 
     # Override the default _libgap_ to use the caching as self._libgap
diff --git a/src/sage/groups/perm_gps/permgroup_named.py b/src/sage/groups/perm_gps/permgroup_named.py
index 2975421e51d..8280062c3a9 100644
--- a/src/sage/groups/perm_gps/permgroup_named.py
+++ b/src/sage/groups/perm_gps/permgroup_named.py
@@ -286,6 +286,11 @@ class SymmetricGroup(PermutationGroup_symalt):
             self._gens = tuple([self.element_class(g, self, check=False)
                                 for g in gens])
 
+    def gap(self):
+        if self._libgap is None:
+            self._libgap = libgap.SymmetricGroup(self._deg)
+        return self._libgap
+
     def _gap_init_(self) -> str:
         """
         Return the string used to create this group in GAP.
@@ -768,6 +773,12 @@ class AlternatingGroup(PermutationGroup_symalt):
         """
         return "Alternating group of order %s!/2 as a permutation group" % self.degree()
 
+    def gap(self):
+        if self._libgap is None:
+            self._libgap = libgap.AlternatingGroup(self._deg)
+        return self._libgap

[github-actions]

Documentation preview for this PR (built with commit ba00a78; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[fchapoton]

please beware of the confusion between gap pexpect interface (that we are trying hard to get rid of), and the libgap library interface, that should be used. Introducing a "gap" method" is not a good thing to do.

[mantepse]

please beware of the confusion between gap pexpect interface (that we are trying hard to get rid of), and the libgap library interface, that should be used. Introducing a "gap" method" is not a good thing to do.

The gap method is already there, I am only removing its double caching!

[fchapoton]

ok, right. At some point, we should get rid of all methods named "gap" and keep instead the alias "_libgap_"

Why are you changing the indentation ?

[mantepse]

Why are you changing the indentation ?

I had to read the code, and understood it better once I indented it the way it is in the commit. Should I undo the indentation? (I don't mind either way.)

[mantepse]

Do you understand why the alternative outlined above could be slower?

[fchapoton]

no need to change back the indentation. And I have no idea about the speed issue

[fchapoton]

ok, let's move forward