Add gappy as a standard package and use for sage's libgap interface

[embray]

Update: The successor to this ticket is #31404 which offers a different approach by using gappy directly in Sage rather than providing Sage-specific wrappers. I think it is overall a better approach for now.

Package tarball: see checksums.ini; use ./configure --enable-download-from-upstream-url to test.

gappy is a new !Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).

It is the result of extracting Sage's sage.libs.gap package into a standalone package without a dependency on Sage, so it still bears some of the legacy of that package (which will continue to be cleaned up and improved on).

One particular improvement is the help system for GAP functions--as before it is possible to extract documentation (when it exists) from the GAP docs, but this is faster now (previously the pexpect interface was still required) and can usually extract only the documentation for the queried function (previously it would also append the entire rest of the documentation chapter). It may also introduce some minor performance improvements in some areas, but it will be interesting to test this with some non-trivial examples.

This ticket is to attempt to convert sage.libs.gap to a wrapper around gappy (rather than replace it entirely). The main reason a wrapper is still needed, is in order to participate in Sage's coercion model.

The GAP interpreter wrapper class sage.libs.gap.libgap.Gap is a Parent, and its elements are instances of GapElement and subclasses thereof which are subclasses of Element.

This presents a challenge which makes wrapping gappy less straightforward than one would hope: Because the classes gappy.Gap and gappy.GapObj (its equivalent of GapElement) are cdef classes, as well as Parent and Element, it is not possible to use multiple inheritance like

class GapElement(GapObj, Element):
    ...

because their base structs are incompatible. The current workaround to this is that GapElement is a wrapper for a GapObj (which in turn wraps C-level GAP Obj pointers). This creates a bit of unfortunate overhead, albeit not much; by using cdef and cpdef methods, Cython can make the call to wrapped objects reasonably fast in many cases.

TODO

• Rework the sage.libs.gap.assigned_names and sage.libs.gap.all_documented_functions modules.

• Fix more uses within sage of the deprecated Gap.function_factory method.

• Fix more tests; all test are passing in sage.libs.gap as well as all in sage.groups.perm_gps, but some tests elsewhere are failing.

• There are miscellaneous other functions in sage.libs.gap that can probably be deprecated or removed.

Food for thought...

There are some other possible workarounds to this issue I would like to explore in the future.

The first is to change how gappy works: Rather than Gap and GapObj being cdef classes, they could be pure-Python wrappers around some cdef classes. This would again create some overhead, but hopefully not too much. Then they can participate in multiple inheritance with Sage's Parent and Element classes.

The other two, which would be much more disruptive, but also possibly useful for other packages that would like to integrate with Sage without explicitly depending on it, and have been discussed before:

1) Create a relatively Sage "base" package with little to no mathematical functionality but that provides base classes for some of Sage's structural classes such as SageObject, Parent, and Element (maybe also CategoryObject?). These base classes need not replicate the full functionality found in Sage, but just enough to provide some bootstrapping. If the package is relatively small and easily pip-installable, packages like gappy can use it as a dependency without requiring all of Sage. See #29865 for an in-progress viable prototype of this.

2) A similar but subtly different option is to make classes like SageObject, Parent, and Element into Abstract Base Classes. Classes in other packages can then be registered as psuedo-subclasses of these ABCs without directly subclassing them, so long as they implement the necessary interfaces. This option is appealing to me since it means classes from other packages can more easily interface with Sage without requiring any additional dependencies. However, it is potentially more disruptive: There isn't a way to ensure that classes which register to an ABC to provide the same C-level interface (C methods and attributes). So Cython-level code that types variables as Parent or Element won't work here, and would have to provide a separate (typically slower) code path for handle ABC pseudo-subclasses. Fortunately there is not a ton of code like this in Sage, but there is some, especially in code related to the coercion model, unsurprisingly.

CC: @tscrim @dimpase @videlec

Component: interfaces

Keywords: gap libgap gappy

Work Issues: release gappy 0.1.0 final and update spkg version before merging

Author: Erik Bray

Branch/Commit: u/embray/gappy @ 82e7e56

Reviewer: Dima Pasechnik

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

[embray]

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-[gappy](https://github.com/embray/gappy) is a new Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).
+[gappy](https://github.com/embray/gappy) is a new !Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).

 It is the result of extracting Sage's `sage.libs.gap` package into a standalone package without a dependency on Sage, so it still bears some of the legacy of that package (which will continue to be cleaned up and improved on).  It may also introduce some minor performance improvements in some areas, but it will be interesting to test this with some non-trivial examples.

[embray]

New commits:

ce1a235

Add gappy as a standard SPKG and dependency of sagelib.

[embray]

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
+**Package tarball:** https://files.pythonhosted.org/packages/b4/1a/78bda94109c877c632dc385b2c2d899e08389bfad89d32b92df0c6636d21/gappy-system-0.1.0a0.tar.gz
+
 [gappy](https://github.com/embray/gappy) is a new !Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).

 It is the result of extracting Sage's `sage.libs.gap` package into a standalone package without a dependency on Sage, so it still bears some of the legacy of that package (which will continue to be cleaned up and improved on).  It may also introduce some minor performance improvements in some areas, but it will be interesting to test this with some non-trivial examples.

[embray]

Branch: u/embray/gappy

[embray]

Author: Erik Bray

[embray]

Commit: ce1a235

[embray]

Description changed:

--- 
+++ 
@@ -2,7 +2,9 @@

 [gappy](https://github.com/embray/gappy) is a new !Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).

-It is the result of extracting Sage's `sage.libs.gap` package into a standalone package without a dependency on Sage, so it still bears some of the legacy of that package (which will continue to be cleaned up and improved on).  It may also introduce some minor performance improvements in some areas, but it will be interesting to test this with some non-trivial examples.
+It is the result of extracting Sage's `sage.libs.gap` package into a standalone package without a dependency on Sage, so it still bears some of the legacy of that package (which will continue to be cleaned up and improved on).
+
+One particular improvement is the help system for GAP functions--as before it is possible to extract documentation (when it exists) from the GAP docs, but this is faster now (previously the pexpect interface was still required) and can usually extract *only* the documentation for the queried function (previously it would also append the entire rest of the documentation chapter). It may also introduce some minor performance improvements in some areas, but it will be interesting to test this with some non-trivial examples.

 This ticket is to attempt to convert `sage.libs.gap` to a wrapper around gappy (rather than replace it entirely).  The main reason a wrapper is still needed, is in order to participate in Sage's coercion model.

[embray]

Description changed:

--- 
+++ 
@@ -19,6 +19,13 @@

 because their base structs are incompatible.  The current workaround to this is that `GapElement` is a wrapper for a `GapObj` (which in turn wraps C-level GAP `Obj` pointers).  This creates a bit of unfortunate overhead, albeit not much; by using `cdef` and `cpdef` methods, Cython can make the call to wrapped objects reasonably fast in many cases.

+## TODO
+
+* Rework the `sage.libs.gap.assigned_names` and `sage.libs.gap.all_documented_functions` modules.
+
+* Fix more uses within sage of the deprecated `Gap.function_factory` method.
+
+* Fix more tests; all test are passing in `sage.libs.gap` as well as all in `sage.groups.perm_gps`, but some tests elsewhere are failing.

 ### Food for thought...

[embray]

Description changed:

--- 
+++ 
@@ -27,6 +27,8 @@

 * Fix more tests; all test are passing in `sage.libs.gap` as well as all in `sage.groups.perm_gps`, but some tests elsewhere are failing.

+* There are miscellaneous other functions in `sage.libs.gap` that can probably be deprecated or removed.
+
 ### Food for thought...

 There are some other possible workarounds to this issue I would like to explore in the future.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from ce1a235 to a8aab2a

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

a8aab2a

Initial work to convert sage.libs.gap to use gappy.

[mkoeppe]

Replying to @embray:

1) Create a relatively Sage "base" package with little to no mathematical functionality but that provides base classes for some of Sage's structural classes such as SageObject, Parent, and Element (maybe also CategoryObject?). These base classes need not replicate the full functionality found in Sage, but just enough to provide some bootstrapping. If the package is relatively small and easily pip-installable, packages like gappy can use it as a dependency without requiring all of Sage.

See #29865 for this

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

45b175e	Don't use the string evaluation construction for libgap permutation
197290f	Fix miscellaneous tests that broke due to different group iteration
9f621e9	Update gappy to v0.1.0a1 with some fixes for matrices and gap_function.
4839316	Miscellaneous updates to not use deprecated interfaces, particularly

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a8aab2a to 4839316

[embray]

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-**Package tarball:** https://files.pythonhosted.org/packages/b4/1a/78bda94109c877c632dc385b2c2d899e08389bfad89d32b92df0c6636d21/gappy-system-0.1.0a0.tar.gz
+**Package tarball:** https://files.pythonhosted.org/packages/9f/86/3571d73c84bf64081413b57d908e516a86ffeb8fcda766919b814220adad/gappy-system-0.1.0a1.tar.gz

 [gappy](https://github.com/embray/gappy) is a new !Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).

[embray]

comment:10

Replying to @mkoeppe:

See #29865 for this

Thanks for pointing it out. I thought I remembered there being a ticket for that. I didn't realize you already had a prototype in the works.

[embray]

comment:11

Travis, I'm curious about your take on this, especially on the impact to permutation groups, since this partly reverts #25609, which I see you worked on. AFAICT this makes some minor performance improvements in construction of permutation groups from Sage -> GAP, but might be a little slower for going from GAP -> Sage.

[embray]

Description changed:

--- 
+++ 
@@ -37,7 +37,7 @@

 The other two, which would be much more disruptive, but also possibly useful for other packages that would like to integrate with Sage without explicitly depending on it, and have been discussed before:

-1) Create a relatively Sage "base" package with little to no mathematical functionality but that provides base classes for some of Sage's structural classes such as `SageObject`, `Parent`, and `Element` (maybe also `CategoryObject`?).  These base classes need not replicate the full functionality found in Sage, but just enough to provide some bootstrapping.  If the package is relatively small and easily pip-installable, packages like gappy can use it as a dependency without requiring all of Sage.
+1) Create a relatively Sage "base" package with little to no mathematical functionality but that provides base classes for some of Sage's structural classes such as `SageObject`, `Parent`, and `Element` (maybe also `CategoryObject`?).  These base classes need not replicate the full functionality found in Sage, but just enough to provide some bootstrapping.  If the package is relatively small and easily pip-installable, packages like gappy can use it as a dependency without requiring all of Sage.  See #29865 for an in-progress viable prototype of this.

 2) A similar but subtly different option is to make classes like `SageObject`, `Parent`, and `Element` into Abstract Base Classes.  Classes in other packages can then be registered as psuedo-subclasses of these ABCs without directly subclassing them, so long as they implement the necessary interfaces.  This option is appealing to me since it means classes from other packages can more easily interface with Sage without requiring any additional dependencies.  However, it is potentially more disruptive: There isn't a way to ensure that classes which register to an ABC to provide the same C-level interface (C methods and attributes).  So Cython-level code that types variables as `Parent` or `Element` won't work here, and would have to provide a separate (typically slower) code path for handle ABC pseudo-subclasses.  Fortunately there is not a ton of code like this in Sage, but there is some, especially in code related to the coercion model, unsurprisingly.

[embray]

Work Issues: release gappy 0.1.0 final and update spkg version before merging

[mkoeppe]

comment:14

Big +1 on this work (but I know too little about GAP to review)

[mkoeppe]

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-**Package tarball:** https://files.pythonhosted.org/packages/9f/86/3571d73c84bf64081413b57d908e516a86ffeb8fcda766919b814220adad/gappy-system-0.1.0a1.tar.gz
+Package tarball: see `checksums.ini`; use `./configure --enable-download-from-upstream-url` to test.

 [gappy](https://github.com/embray/gappy) is a new !Python/Cython package providing a Python interface to GAP using its public "libgap" API (to the extent possible).

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4839316 to baa0f61

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

baa0f61

Update gappy to v0.1.0a2 which adds MacOS and Cygwin fixes.

[mkoeppe]

comment:16

A quick remark on install-requires: For prerelease versions, it's best to use the exact version; version ranges may not work (see #30913 comment:81), so it should be gappy ==0.1.0a0

[videlec]

comment:17

gappy is an excellent news!

I would like to bring the attention to cypari2 that played the role of gappy at the time the PARI/GP interface in Sage became an independent Cython module. In particular, if any design decision has to be made, it would be weird if they would be different for both interfaces.

First of all, two things that seem different from the rough description of the ticket

• there is no extra layer between cypari2 and Sage. Sage deals with cypari2.gen.Gen objects directly.
• cypari2 is completely agnostic to Sage and its coercion system

There was no need for coercion because the most basic possible happen : in any mixture, everything is converted to a cypari2 Gen object. Here with integers

sage: type(1 + pari(1))
<class 'cypari2.gen.Gen'>
sage: type(pari(1) + 1)
<class 'cypari2.gen.Gen'>

Here with matrices

sage: type(pari("[1, 2; 3, 4]") + matrix(2, [1,1,1,1]))
<class 'cypari2.gen.Gen'>
sage: type(matrix(2, [1,1,1,1]) + pari("[1, 2; 3, 4]"))
<class 'cypari2.gen.Gen'>

I do not see the difference here with the GAP interface. In what sort of construction the interaction with categories/coercion has to be handled? I would like a more concrete description if possible.

On a related note, cypari2 introduced the standardized name __pari__ for the method that would provide a conversion of any object to its cypari2 equivalent. That way, any Python library could make interaction with cypari2.

For the cypari2 -> Sage conversion, there is sage.libs.pari.convert_sage.gen_to_sage which is a partial but efficient converter. It introduced a slowdown in conversions PARI/GP -> Sage because the sage method on cypari2.gen.Gen (which was kept) became

cdef class Gen:
    def sage(self, locals=None):
        r"""
        Return the closest SageMath equivalent of the given PARI object.

        INPUT:

        - ``locals`` -- optional dictionary used in fallback cases that
          involve ``sage_eval``

        See :func:`~sage.libs.pari.convert_sage.gen_to_sage` for more information.
        """
        from sage.libs.pari.convert_sage import gen_to_sage
        return gen_to_sage(self, locals)

[tscrim]

comment:18

Replying to @embray:

Travis, I'm curious about your take on this, especially on the impact to permutation groups, since this partly reverts #25609, which I see you worked on. AFAICT this makes some minor performance improvements in construction of permutation groups from Sage -> GAP, but might be a little slower for going from GAP -> Sage.

A good stress test would be iterating through the E₇ Weyl group:

sage: W = CoxeterGroup('E7', implementation='permutation')
sage: W.cardinality()                                                                                                     
2903040
sage: %time for w in W.iteration('depth', False): pass                                                                    
CPU times: user 1.05 s, sys: 391 µs, total: 1.05 s
Wall time: 1.05 s

If you are more brave, do E₈ (which has 696729600 elements), but that will take many minutes to complete. D₉ is perhaps a better intermediate example:

sage: W = CoxeterGroup('D9', implementation='permutation')                                                                
sage: W.cardinality()                                                                                                     
92897280
sage: %time for w in W.iteration('depth', False): pass                                                                    
CPU times: user 34.4 s, sys: 0 ns, total: 34.4 s
Wall time: 34.4 s

As long as the slowdown is relatively small, then I think this is a good way forward as it makes maintenance easier. Plus it will benefit the broader community.

[embray]

comment:19

Replying to @tscrim:

Replying to @embray:

Travis, I'm curious about your take on this, especially on the impact to permutation groups, since this partly reverts #25609, which I see you worked on. AFAICT this makes some minor performance improvements in construction of permutation groups from Sage -> GAP, but might be a little slower for going from GAP -> Sage.

A good stress test would be iterating through the E₇ Weyl group:

I got,

before:

sage: W = CoxeterGroup('E7', implementation='permutation')                                                                                                                                       
sage: W.cardinality()                                                                                                                                                                            
2903040
sage: %time for w in W.iteration('depth', False): pass                                                                                                                                           
CPU times: user 1.27 s, sys: 518 µs, total: 1.27 s
Wall time: 1.27 s
sage: W = CoxeterGroup('D9', implementation='permutation')                                                                                                                                       
sage: W.cardinality()                                                                                                                                                                            
92897280
sage: %time for w in W.iteration('depth', False): pass                                                                                                                                           
CPU times: user 42.7 s, sys: 40.9 ms, total: 42.7 s
Wall time: 42.8 s

after:

sage: W = CoxeterGroup('E7', implementation='permutation')                                                                                                                                       
sage: W.cardinality()                                                                                                                                                                            
2903040
sage: %time for w in W.iteration('depth', False): pass                                                                                                                                           
CPU times: user 1.26 s, sys: 0 ns, total: 1.26 s
Wall time: 1.27 s
sage: %timeit for w in W.iteration('depth', False): pass                                                                                                                                         
1.23 s ± 64.9 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: W = CoxeterGroup('D9', implementation='permutation')                                                                                                                                       
sage: W.cardinality()                                                                                                                                                                            
92897280
sage: %time for w in W.iteration('depth', False): pass                                                                                                                                           
CPU times: user 44.7 s, sys: 3.91 ms, total: 44.7 s
Wall time: 44.7 s
sage: %timeit for w in W.iteration('depth', False): pass                                                                                                                                         
42.7 s ± 1.24 s per loop (mean ± std. dev. of 7 runs, 1 loop each)

so at least for this case no better but no worse.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

a07f20e

Fixes needed for gappy v0.1.0a2 which did away with the libgap_soname

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from baa0f61 to a07f20e

[embray]

comment:21

Replying to @videlec:

gappy is an excellent news!

I would like to bring the attention to cypari2 that played the role of gappy at the time the PARI/GP interface in Sage became an independent Cython module. In particular, if any design decision has to be made, it would be weird if they would be different for both interfaces.

Thanks for your comments Vincent. Yes, this is definitely intended in the same vein as cypari2.

First of all, two things that seem different from the rough description of the ticket

• there is no extra layer between cypari2 and Sage. Sage deals with cypari2.gen.Gen objects directly.
• cypari2 is completely agnostic to Sage and its coercion system

I think this is what I would prefer as well.

There was no need for coercion because the most basic possible happen : in any mixture, everything is converted to a cypari2 Gen object. Here with integers

sage: type(1 + pari(1))
<class 'cypari2.gen.Gen'>
sage: type(pari(1) + 1)
<class 'cypari2.gen.Gen'>

Here with matrices

sage: type(pari("[1, 2; 3, 4]") + matrix(2, [1,1,1,1]))
<class 'cypari2.gen.Gen'>
sage: type(matrix(2, [1,1,1,1]) + pari("[1, 2; 3, 4]"))
<class 'cypari2.gen.Gen'>

Here's the same examples using gappy directly. The integers work:

sage: from gappy import gap                                                                                                                                                                      
sage: type(1 + gap(1))                                                                                                                                                                           
<class 'gappy.gapobj.GapInteger'>
sage: type(gap(1) + 1)                                                                                                                                                                           
<class 'gappy.gapobj.GapInteger'>

but the matrices do not for some reason; I think because gappy uses a _gap_ magic method (similarly to __pari__, but this conflicts with Sage's existing _gap_ magic method for using the GAP pexpect interface). I get:

sage: type(gap([[1, 2], [3, 4]]) + matrix(2, [1, 1, 1, 1]))
AttributeError                            Traceback (most recent call last)
<ipython-input-4-6fdcc2d39852> in <module>
----> 1 type(gap([[Integer(1), Integer(2)], [Integer(3), Integer(4)]]) + matrix(Integer(2), [Integer(1), Integer(1), Integer(1), Integer(1)]))

gappy/gapobj.pyx in gappy.gapobj.GapObj.__add__()

gappy/gapobj.pyx in gappy.gapobj.GapObj.parent()

gappy/core.pyx in gappy.core.Gap.__call__()

~/src/sagemath/sage/local/lib/python3.6/site-packages/sage/structure/sage_object.pyx in sage.structure.sage_object.SageObject._gap_ (build/cythonized/sage/structure/sage_object.c:6225)()
    714             import sage.interfaces.gap
    715             G = sage.interfaces.gap.gap
--> 716         return self._interface_(G)
    717 
    718     def _gap_init_(self):

~/src/sagemath/sage/local/lib/python3.6/site-packages/sage/structure/sage_object.pyx in sage.structure.sage_object.SageObject._interface_ (build/cythonized/sage/structure/sage_object.c:5480)()
    678             except (KeyError, ValueError):
    679                 pass
--> 680         nm = I.name()
    681         init_func = getattr(self, '_%s_init_' % nm, None)
    682         if init_func is not None:

gappy/core.pyx in gappy.core.Gap.__getattr__()

AttributeError: no GAP global variable bound to 'name'

However, if I do this:

sage: gap = libgap.gap  # This is the gappy.Gap instance wrapped by Sage's interface                                                                                                                                                                    
sage: gap
<Gap(gap_root='/home/embray/src/sagemath/sage/local/share/gap')>
sage: type(gap([[1, 2], [3, 4]]) + matrix(2, [1, 1, 1, 1]))                                                                                                                                      
<class 'gappy.gapobj.GapList'>
sage: gap([[1, 2], [3, 4]]) + matrix(2, [1, 1, 1, 1])                                                                                                                                            
[ [ 2, 3 ], [ 4, 5 ] ]
sage: type(matrix(2, [1, 1, 1, 1]) + gap([[1, 2], [3, 4]]))                                                                                                                                      
<class 'gappy.gapobj.GapList'>
sage: matrix(2, [1, 1, 1, 1]) + gap([[1, 2], [3, 4]])                                                                                                                                            
[ [ 2, 3 ], [ 4, 5 ] ]

It has the necessary converters registered to handle SageObjects correctly.

So it looks like gappy and cypari2 are indeed mostly similar in this case.

I do not see the difference here with the GAP interface. In what sort of construction the interaction with categories/coercion has to be handled? I would like a more concrete description if possible.

Honestly, I do not know. I don't even understand the coercion system well-enough to say. It's just because the old sage.libs.gap did use it, so I tried, for now, to keep the same interface for a couple reasons:

• Backwards compatibility--I didn't want to break the existing API too much, especially if someone is writing code that explicitly checks for Sage GapElements. I do not know if anyone is actually doing this.

• Consistency--the other Sage interfaces to other languages use the Parent/Element model so I just tried to keep that consistent. Maybe it is not really necessary or useful though. If someone can confirm for me that it isn't then we can use gappy more-or-less directly and do a way with a lot of extra complexity.

On a related note, cypari2 introduced the standardized name __pari__ for the method that would provide a conversion of any object to its cypari2 equivalent. That way, any Python library could make interaction with cypari2.

For the cypari2 -> Sage conversion, there is sage.libs.pari.convert_sage.gen_to_sage which is a partial but efficient converter. It introduced a slowdown in conversions PARI/GP -> Sage because the sage method on cypari2.gen.Gen (which was kept) became

This is one area where I think it's still useful to have wrapper classes in Sage; which is just to add a .sage() methods to gappy GapObjs. But if they don't have to be Element subclasses, then they can simply subclass GapObj rather than wrap them.

---

New commits:

a07f20e

Fixes needed for gappy v0.1.0a2 which did away with the libgap_soname

[embray]

comment:22

Replying to @embray:

This is one area where I think it's still useful to have wrapper classes in Sage; which is just to add a .sage() methods to gappy GapObjs. But if they don't have to be Element subclasses, then they can simply subclass GapObj rather than wrap them.

Another possibility is that just as user classes can have a _gap_ method to convert to GAP-compatible representations, gappy could add a system for registering "GAP -> " converters. They could have .sage() methods tacked on to them without having to make subclasses.

[videlec]

comment:23

Replying to @embray:

Replying to @videlec:

gappy is an excellent news!

I would like to bring the attention to cypari2 that played the role of gappy at the time the PARI/GP interface in Sage became an independent Cython module. In particular, if any design decision has to be made, it would be weird if they would be different for both interfaces.

Thanks for your comments Vincent. Yes, this is definitely intended in the same vein as cypari2.

First of all, two things that seem different from the rough description of the ticket

• there is no extra layer between cypari2 and Sage. Sage deals with cypari2.gen.Gen objects directly.
• cypari2 is completely agnostic to Sage and its coercion system

I think this is what I would prefer as well.

Could we try to make the direct interface work as well? I mean as far as binary operators are concerned, it should be fine to mix gappy elements with SageMath elements. If an intermediate layer is needed it could also be introduced. I think gappy is a good occasion to clean the design of the gap interface.

There was no need for coercion because the most basic possible happen : in any mixture, everything is converted to a cypari2 Gen object. Here with integers

sage: type(1 + pari(1))
<class 'cypari2.gen.Gen'>
sage: type(pari(1) + 1)
<class 'cypari2.gen.Gen'>

Here with matrices

sage: type(pari("[1, 2; 3, 4]") + matrix(2, [1,1,1,1]))
<class 'cypari2.gen.Gen'>
sage: type(matrix(2, [1,1,1,1]) + pari("[1, 2; 3, 4]"))
<class 'cypari2.gen.Gen'>

Here's the same examples using gappy directly. The integers work:

sage: from gappy import gap
sage: type(1 + gap(1))
<class 'gappy.gapobj.GapInteger'>
sage: type(gap(1) + 1)
<class 'gappy.gapobj.GapInteger'>

but the matrices do not for some reason; I think because gappy uses a _gap_ magic method (similarly to __pari__, but this conflicts with Sage's existing _gap_ magic method for using the GAP pexpect interface). I get:

sage: type(gap([[1, 2], [3, 4]]) + matrix(2, [1, 1, 1, 1]))
AttributeError                            Traceback (most recent call last)
<ipython-input-4-6fdcc2d39852> in <module>
----> 1 type(gap([[Integer(1), Integer(2)], [Integer(3), Integer(4)]]) + matrix(Integer(2), [Integer(1), Integer(1), Integer(1), Integer(1)]))

gappy/gapobj.pyx in gappy.gapobj.GapObj.__add__()

gappy/gapobj.pyx in gappy.gapobj.GapObj.parent()

gappy/core.pyx in gappy.core.Gap.__call__()

~/src/sagemath/sage/local/lib/python3.6/site-packages/sage/structure/sage_object.pyx in sage.structure.sage_object.SageObject._gap_ (build/cythonized/sage/structure/sage_object.c:6225)()
    714             import sage.interfaces.gap
    715             G = sage.interfaces.gap.gap
--> 716         return self._interface_(G)
    717 
    718     def _gap_init_(self):

~/src/sagemath/sage/local/lib/python3.6/site-packages/sage/structure/sage_object.pyx in sage.structure.sage_object.SageObject._interface_ (build/cythonized/sage/structure/sage_object.c:5480)()
    678             except (KeyError, ValueError):
    679                 pass
--> 680         nm = I.name()
    681         init_func = getattr(self, '_%s_init_' % nm, None)
    682         if init_func is not None:

gappy/core.pyx in gappy.core.Gap.__getattr__()

AttributeError: no GAP global variable bound to 'name'

However, if I do this:

sage: gap = libgap.gap  # This is the gappy.Gap instance wrapped by Sage's interface                                                                                                                                                                    
sage: gap
<Gap(gap_root='/home/embray/src/sagemath/sage/local/share/gap')>
sage: type(gap([[1, 2], [3, 4]]) + matrix(2, [1, 1, 1, 1]))                                                                                                                                      
<class 'gappy.gapobj.GapList'>
sage: gap([[1, 2], [3, 4]]) + matrix(2, [1, 1, 1, 1])                                                                                                                                            
[ [ 2, 3 ], [ 4, 5 ] ]
sage: type(matrix(2, [1, 1, 1, 1]) + gap([[1, 2], [3, 4]]))                                                                                                                                      
<class 'gappy.gapobj.GapList'>
sage: matrix(2, [1, 1, 1, 1]) + gap([[1, 2], [3, 4]])                                                                                                                                            
[ [ 2, 3 ], [ 4, 5 ] ]

It has the necessary converters registered to handle SageObjects correctly.

So it looks like gappy and cypari2 are indeed mostly similar in this case.

I do not see the difference here with the GAP interface. In what sort of construction the interaction with categories/coercion has to be handled? I would like a more concrete description if possible.

Honestly, I do not know. I don't even understand the coercion system well-enough to say. It's just because the old sage.libs.gap did use it, so I tried, for now, to keep the same interface for a couple reasons:

• Backwards compatibility--I didn't want to break the existing API too much, especially if someone is writing code that explicitly checks for Sage GapElements. I do not know if anyone is actually doing this.

• Consistency--the other Sage interfaces to other languages use the Parent/Element model so I just tried to keep that consistent. Maybe it is not really necessary or useful though. If someone can confirm for me that it isn't then we can use gappy more-or-less directly and do a way with a lot of extra complexity.

hmm. PARI/GP is a counterexemple to that.

One construction I can imagine where the layer could be relevant is if you want to use some gap objects as coefficients of a matrix. To do that, you would use

sage: MatrixSpace(GapBaseRing(), 3, 3)

where GapBaseRing() would be a GapObj modelling a ring of numbers. Then if GapBaseRing() is not recognized as a proper ring from SageMath such construction would be impossible. I am not sure it is currently used anywhere.

On a related note, cypari2 introduced the standardized name __pari__ for the method that would provide a conversion of any object to its cypari2 equivalent. That way, any Python library could make interaction with cypari2.

Actually one could use __gap__ (double underscore) for the gappy objects and keep the single underscore _gap_/_libgap_ for the one with coercion layer. The double score convention is more Pythonic and, I think, prefered outside of SageMath. Moreover, I believe the simple underscore could be as generic as

file: src/sage/structure/element.pyx

cdef class Element(SageObject):
    def _gap_(self):
        from sage.libs.gap.wrapper import GappyWrapperWithCoercion
        return GappyWrapperWithCoercion(self.__gap__())

For the cypari2 -> Sage conversion, there is sage.libs.pari.convert_sage.gen_to_sage which is a partial but efficient converter. It introduced a slowdown in conversions PARI/GP -> Sage because the sage method on cypari2.gen.Gen (which was kept) became

This is one area where I think it's still useful to have wrapper classes in Sage; which is just to add a .sage() methods to gappy GapObjs. But if they don't have to be Element subclasses, then they can simply subclass GapObj rather than wrap them.

[videlec]

comment:24

Replying to @embray:

Replying to @embray:

This is one area where I think it's still useful to have wrapper classes in Sage; which is just to add a .sage() methods to gappy GapObjs. But if they don't have to be Element subclasses, then they can simply subclass GapObj rather than wrap them.

Another possibility is that just as user classes can have a _gap_ method to convert to GAP-compatible representations, gappy could add a system for registering "GAP -> " converters. They could have .sage() methods tacked on to them without having to make subclasses.

Having something flexible (ie runtime registration) looks like a better deal. Still what would be the main converter call? Whatever system could argue for having GapObj.my_super_system_conversion() in gappy. Or you wanted the GapObj.my_super_system_conversion() to be created at runtime?

[embray]

comment:25

Replying to @videlec:

Actually one could use __gap__ (double underscore) for the gappy objects and keep the single underscore _gap_/_libgap_ for the one with coercion layer. The double score convention is more Pythonic and, I think, prefered outside of SageMath.

I was explicitly avoiding this for gappy since the Python documentation discourages use of custom dunder methods. I believe we had some bikeshedding about this with __pari__ as well, though I don't actually recall what side I fell on at that time.

On the other hand, it would be nice and consistent with __pari__, and it seems pretty unlikely that Python will use the name __gap__ for anything any time soon (though stranger things have happened). Numpy also has a long history of using custom dunder methods, though it prefixes most of them with __numpy_.

Will respond to the rest of the comment later.

[embray]

comment:26

Replying to @videlec:

Replying to @embray:

Replying to @embray:

This is one area where I think it's still useful to have wrapper classes in Sage; which is just to add a .sage() methods to gappy GapObjs. But if they don't have to be Element subclasses, then they can simply subclass GapObj rather than wrap them.

Another possibility is that just as user classes can have a _gap_ method to convert to GAP-compatible representations, gappy could add a system for registering "GAP -> " converters. They could have .sage() methods tacked on to them without having to make subclasses.

Having something flexible (ie runtime registration) looks like a better deal. Still what would be the main converter call? Whatever system could argue for having GapObj.my_super_system_conversion() in gappy. Or you wanted the GapObj.my_super_system_conversion() to be created at runtime?

The new version I just pushed shows some examples of how it works: https://gappy.readthedocs.io/en/latest/api.html#gappy.gapobj.GapObj.convert_to

See in particular the example of

@GapList.convert_to('numpy')

which registers a converter from GapList to Numpy arrays. Same could be done for GapFoo.convert_to('sage').

[embray]

comment:27

One simple example which no longer works without the coercion model is:

sage: from gappy import gap                                                                                                                                                                      
sage: one = gap(1)                                                                                                                                                                               
sage: type(one)                                                                                                                                                                                  
<class 'gappy.gapobj.GapInteger'>
sage: Integer(one)                                                                                                                                                                               
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-1860a9d72d2d> in <module>
----> 1 Integer(one)

~/src/sagemath/sage/local/lib/python3.6/site-packages/sage/rings/integer.pyx in sage.rings.integer.Integer.__init__ (build/cythonized/sage/rings/integer.c:7078)()
    745                     return
    746 
--> 747                 raise TypeError("unable to coerce %s to an integer" % type(x))
    748 
    749     def __reduce__(self):

TypeError: unable to coerce <class 'gappy.gapobj.GapInteger'> to an integer

I'm sure this has an easy solution but I don't know what the best approach would be.

[videlec]

comment:28

Replying to @embray:

One simple example which no longer works without the coercion model is:

sage: from gappy import gap                                                                                                                                                                      
sage: one = gap(1)                                                                                                                                                                               
sage: type(one)                                                                                                                                                                                  
<class 'gappy.gapobj.GapInteger'>
sage: Integer(one)                                                                                                                                                                               
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-1860a9d72d2d> in <module>
----> 1 Integer(one)

~/src/sagemath/sage/local/lib/python3.6/site-packages/sage/rings/integer.pyx in sage.rings.integer.Integer.__init__ (build/cythonized/sage/rings/integer.c:7078)()
    745                     return
    746 
--> 747                 raise TypeError("unable to coerce %s to an integer" % type(x))
    748 
    749     def __reduce__(self):

TypeError: unable to coerce <class 'gappy.gapobj.GapInteger'> to an integer

I'm sure this has an easy solution but I don't know what the best approach would be.

Integer constructor has a special case for cypari2

file: src/sage/rings/integer.pyx
cdef class Integer(Element):
    def __init__(self, x):
        ...
        elif isinstance(x, pari_gen):
            set_from_pari_gen(self, x)

This is the general approach taken by cypari2. Though, maybe it is not desirable to clutter the sage element constructors.

[embray]

comment:29

Maybe just as there is __pari__ and __gap__ we should develop a standard for a __sage__ magic method that Sage can call on objects from other libraries in order to get their equivalent representation as a Sage type.

For now I could add a special case in the Integer constructor but I agree it's not ideal. I noticed it's also the case in the permutation element

cdef class PermutationGroupElement(MultiplicativeGroupElement):
    ...
    def __init__(self, g, parent, check=True):
        ...
        elif isinstance(g, GapElement):
            if isinstance(g, GapElement_Permutation):
                self._set_libgap(g.obj)
            elif isinstance(g, GapElement_String):
                self._set_string(g.sage())
            elif isinstance(g, GapElement_List):
                self._set_list_images(g.sage(), convert)
            else:
                raise ValueError("invalid data to initialize a permutation")
        ...

So, nothing very "magic" going on...

[embray]

comment:30

Just for the record, another idea I considered is providing an interface to gappy to allow it to replace the base class for objects returned by it, sort of like how Sage Parents have an Element attribute, it would be possible to subclass gappy.Gap and provide a different base class. This would also have a mechanism for instantiating the correct subclass of the base class (e.g. GapPermutation or GapElement_Permutation).

However, I don't know that this has much of a use case outside of Sage. For now I'm working on a more disruptive approach inspired by the cypari2 conversion to just use gappy more directly and do away with GapElement.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c9160c1

Further rework of 45b175e43621795f890204a9cdfb30b524f961fa which

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a07f20e to c9160c1

[mkoeppe]

comment:32

Replying to @embray:

For now I could add a special case in the Integer constructor but I agree it's not ideal.

Just a quick note that the compile-time dependency of sage.rings.integer on various libraries will be a major obstacle to modularization (#29705), see #30022. This will become relevant later (perhaps in the Sage 9.5 cycle according to current plans), but it won't block immediate development, so I have no objections.

[videlec]

comment:33

Replying to @mkoeppe:

Replying to @embray:

For now I could add a special case in the Integer constructor but I agree it's not ideal.

Just a quick note that the compile-time dependency of sage.rings.integer on various libraries will be a major obstacle to modularization (#29705), see #30022. This will become relevant later (perhaps in the Sage 9.5 cycle according to current plans), but it won't block immediate development, so I have no objections.

Good point. The remark applies equally to gappy and cypari2 (and possibly other libraries).

In the same vein that Erik proposed in [comment:26] we could introduce a way to register conversions to Sage so that ZZ(my_cypari2_object) (construction from the parent) works. One would then drop the support for Integer(my_cypari2_object) (construction from the element class). There will be some time penalty in doing this. But fast conversion functions would remain available in sage.libs.pari.

On the other hand, Integer does implement a __pari__ method for conversion to cypari2. That should be changed at the same time.

[videlec]

comment:34

Replying to @videlec:

Replying to @mkoeppe:

Replying to @embray:

For now I could add a special case in the Integer constructor but I agree it's not ideal.

Just a quick note that the compile-time dependency of sage.rings.integer on various libraries will be a major obstacle to modularization (#29705), see #30022. This will become relevant later (perhaps in the Sage 9.5 cycle according to current plans), but it won't block immediate development, so I have no objections.

Good point. The remark applies equally to gappy and cypari2 (and possibly other libraries).

In the same vein that Erik proposed in [comment:26] we could introduce a way to register conversions to Sage so that ZZ(my_cypari2_object) (construction from the parent) works. One would then drop the support for Integer(my_cypari2_object) (construction from the element class). There will be some time penalty in doing this. But fast conversion functions would remain available in sage.libs.pari.

On the other hand, Integer does implement a __pari__ method for conversion to cypari2. That should be changed at the same time.

Beyond conversions, many methods of Integer depend exlicitely on __pari__

• perfect_power
• is_prime_power
• is_prime
• next_probable_prime
• next_prime
• previous_prime
• etc Given that, I don't see how we could make Integer independent of cypari2 without removing most of it functionalities.

[mkoeppe]

comment:35

These are just Python methods, not cdef's, right? I think they can just be patched into the class in the same way that https://pypi.org/project/recursive-monkey-patch/ does

Crucial for the modularization is really just that the compiled Cython module no longer has such compile-time dependencies.

[videlec]

comment:36

Replying to @mkoeppe:

These are just Python methods, not cdef's, right? I think they can just be patched into the class in the same way that https://pypi.org/project/recursive-monkey-patch/ does

Crucial for the modularization is really just that the compiled Cython module no longer has such compile-time dependencies.

Not really. Integer is an extension class, hence not extensible by any mean.

[mkoeppe]

comment:37

Thanks, you are right, of course.

[mkoeppe]

comment:38

On the other hand Element.__getattribute__ already dispatches fairly dynamically via getattr_from_category, so...

[tscrim]

comment:39

Replying to @mkoeppe:

On the other hand Element.__getattribute__ already dispatches fairly dynamically via getattr_from_category, so...

The problem with that is it is relatively slow IIRC. It might be better to have a Python subclass of Integer that can be monkey patched.

[embray]

comment:40

Replying to @mkoeppe:

Replying to @embray:

For now I could add a special case in the Integer constructor but I agree it's not ideal.

Just a quick note that the compile-time dependency of sage.rings.integer on various libraries will be a major obstacle to modularization (#29705), see #30022. This will become relevant later (perhaps in the Sage 9.5 cycle according to current plans), but it won't block immediate development, so I have no objections.

I wonder if a minimal Integer base type wouldn't also be helpful here. Again all we need are base classes that have the same struct layout, and maybe a minimum of methods defined.