Opened 23 months ago
Closed 21 months ago
#31297 closed enhancement (wontfix)
Add gappy as a standard package and use for sage's libgap interface
Reported by:  Erik Bray  Owned by:  

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  interfaces  Keywords:  gap libgap gappy 
Cc:  Travis Scrimshaw, Dima Pasechnik, Vincent Delecroix  Merged in:  
Authors:  Erik Bray  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  release gappy 0.1.0 final and update spkg version before merging 
Branch:  u/embray/gappy (Commits, GitHub, GitLab)  Commit:  82e7e56cdea9b8d24a3465f9661189288f54cc26 
Dependencies:  Stopgaps: 
Description (last modified by )
Update: The successor to this ticket is #31404 which offers a different approach by using gappy directly in Sage rather than providing Sagespecific wrappers. I think it is overall a better approach for now.
Package tarball: see checksums.ini
; use ./configure enabledownloadfromupstreamurl
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 functionsas 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 nontrivial 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 Clevel 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
andsage.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 insage.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 purePython 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 pipinstallable, packages like gappy can use it as a dependency without requiring all of Sage. See #29865 for an inprogress 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 psuedosubclasses 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 Clevel interface (C methods and attributes). So Cythonlevel 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 pseudosubclasses. 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.
Change History (50)
comment:1 Changed 23 months ago by
Description:  modified (diff) 

comment:2 Changed 23 months ago by
Authors:  → Erik Bray 

Branch:  → u/embray/gappy 
Commit:  → ce1a235e093aa42c0cab6a0e9a5e80e7fc92ee84 
Description:  modified (diff) 
comment:3 Changed 23 months ago by
Description:  modified (diff) 

comment:4 Changed 23 months ago by
Description:  modified (diff) 

comment:5 Changed 23 months ago by
Description:  modified (diff) 

comment:6 Changed 23 months ago by
Commit:  ce1a235e093aa42c0cab6a0e9a5e80e7fc92ee84 → a8aab2adfe417087af7f1ad5393ca7a61e242613 

Branch pushed to git repo; I updated commit sha1. New commits:
a8aab2a  Initial work to convert sage.libs.gap to use gappy.

comment:7 followup: 10 Changed 23 months ago by
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
, andElement
(maybe alsoCategoryObject
?). 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 pipinstallable, packages like gappy can use it as a dependency without requiring all of Sage.
See #29865 for this
comment:8 Changed 22 months ago by
Commit:  a8aab2adfe417087af7f1ad5393ca7a61e242613 → 483931628b3a4ce7ffe9e15fb28de20b5362a757 

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

comment:9 Changed 22 months ago by
Description:  modified (diff) 

comment:10 Changed 22 months ago by
comment:11 followup: 18 Changed 22 months ago by
Cc:  Travis Scrimshaw added 

Status:  new → needs_review 
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.
comment:12 Changed 22 months ago by
Description:  modified (diff) 

comment:13 Changed 22 months ago by
Work issues:  → release gappy 0.1.0 final and update spkg version before merging 

comment:14 Changed 22 months ago by
Cc:  Dima Pasechnik added 

Description:  modified (diff) 
Big +1 on this work (but I know too little about GAP to review)
comment:15 Changed 22 months ago by
Commit:  483931628b3a4ce7ffe9e15fb28de20b5362a757 → baa0f61f86120ca2bb53e6d3a039f69239a1d0ac 

Branch pushed to git repo; I updated commit sha1. New commits:
baa0f61  Update gappy to v0.1.0a2 which adds MacOS and Cygwin fixes.

comment:16 Changed 22 months ago by
A quick remark on installrequires
: For prerelease versions, it's best to use the exact version; version ranges may not work (see
https://trac.sagemath.org/ticket/30913#comment:81),
so it should be gappy ==0.1.0a0
comment:17 followup: 21 Changed 22 months ago by
Cc:  Vincent Delecroix added 

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)
comment:18 followup: 19 Changed 22 months ago by
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_{7} 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_{8} (which has 696729600 elements), but that will take many minutes to complete. D_{9} 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.
comment:19 Changed 22 months ago by
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_{7} 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.
comment:20 Changed 22 months ago by
Commit:  baa0f61f86120ca2bb53e6d3a039f69239a1d0ac → a07f20ecee90d82914116e45b31e58b93d0e3ca1 

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

comment:21 followups: 22 23 Changed 22 months ago by
Replying to vdelecroix:
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) <ipythoninput46fdcc2d39852> 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/sitepackages/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/sitepackages/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 SageObject
s 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 wellenough 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 compatibilityI didn't want to break the existing API too much, especially if someone is writing code that explicitly checks for Sage
GapElement
s. I do not know if anyone is actually doing this.
 Consistencythe 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 moreorless 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 thesage
method oncypari2.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 GapObj
s. 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

comment:22 followup: 24 Changed 22 months ago by
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 gappyGapObj
s. But if they don't have to beElement
subclasses, then they can simply subclassGapObj
rather than wrap them.
Another possibility is that just as user classes can have a _gap_
method to convert to GAPcompatible representations, gappy could add a system for registering "GAP > <whatever>" converters. They could have .sage()
methods tacked on to them without having to make subclasses.
comment:23 followup: 25 Changed 22 months ago by
Replying to embray:
Replying to vdelecroix:
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) <ipythoninput46fdcc2d39852> 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/sitepackages/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/sitepackages/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
SageObject
s 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 wellenough 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 compatibilityI didn't want to break the existing API too much, especially if someone is writing code that explicitly checks for Sage
GapElement
s. I do not know if anyone is actually doing this.
 Consistencythe 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 moreorless 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 thesage
method oncypari2.gen.Gen
(which was kept) becameThis 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 gappyGapObj
s. But if they don't have to beElement
subclasses, then they can simply subclassGapObj
rather than wrap them.
comment:24 followup: 26 Changed 22 months ago by
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 gappyGapObj
s. But if they don't have to beElement
subclasses, then they can simply subclassGapObj
rather than wrap them.Another possibility is that just as user classes can have a
_gap_
method to convert to GAPcompatible representations, gappy could add a system for registering "GAP > <whatever>" 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?
comment:25 Changed 22 months ago by
Replying to vdelecroix:
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.
comment:26 Changed 22 months ago by
Replying to vdelecroix:
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 gappyGapObj
s. But if they don't have to beElement
subclasses, then they can simply subclassGapObj
rather than wrap them.Another possibility is that just as user classes can have a
_gap_
method to convert to GAPcompatible representations, gappy could add a system for registering "GAP > <whatever>" 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 theGapObj.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')
.
comment:27 followup: 28 Changed 22 months ago by
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) <ipythoninput41860a9d72d2d> in <module> > 1 Integer(one) ~/src/sagemath/sage/local/lib/python3.6/sitepackages/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.
comment:28 Changed 22 months ago by
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) <ipythoninput41860a9d72d2d> in <module> > 1 Integer(one) ~/src/sagemath/sage/local/lib/python3.6/sitepackages/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 integerI'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.
comment:29 followup: 32 Changed 22 months ago by
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...
comment:30 Changed 22 months ago by
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
.
comment:31 Changed 22 months ago by
Commit:  a07f20ecee90d82914116e45b31e58b93d0e3ca1 → c9160c10e033565c1e2b538cb55f89f4c518ae72 

Branch pushed to git repo; I updated commit sha1. New commits:
c9160c1  Further rework of 45b175e43621795f890204a9cdfb30b524f961fa which

comment:32 followups: 33 40 Changed 22 months ago by
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 compiletime 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.
comment:33 followup: 34 Changed 22 months ago by
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 compiletime 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 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.
comment:34 Changed 22 months ago by
Replying to vdelecroix:
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 compiletime 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 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 forInteger(my_cypari2_object)
(construction from the element class). There will be some time penalty in doing this. But fast conversion functions would remain available insage.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.
comment:35 followup: 36 Changed 22 months ago by
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/recursivemonkeypatch/ does
Crucial for the modularization is really just that the compiled Cython module no longer has such compiletime dependencies.
comment:36 followup: 41 Changed 22 months ago by
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/recursivemonkeypatch/ doesCrucial for the modularization is really just that the compiled Cython module no longer has such compiletime dependencies.
Not really. Integer
is an extension class, hence not extensible by any mean.
comment:38 followup: 39 Changed 22 months ago by
On the other hand Element.__getattribute__
already dispatches fairly dynamically via getattr_from_category
, so...
comment:39 Changed 22 months ago by
Replying to mkoeppe:
On the other hand
Element.__getattribute__
already dispatches fairly dynamically viagetattr_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.
comment:40 Changed 22 months ago by
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 compiletime 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.
comment:41 Changed 22 months ago by
Replying to vdelecroix:
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/recursivemonkeypatch/ doesCrucial for the modularization is really just that the compiled Cython module no longer has such compiletime dependencies.
Not really.
Integer
is an extension class, hence not extensible by any mean.
No, but a plain Python subclass of it is.
comment:42 Changed 22 months ago by
Another important example that doesn't work without coercion support which did before is evaluation of polynomials on GAP objects:
sage: from gappy import gap sage: R.<x> = PolynomialRing(ZZ) sage: x = R.gen() sage: x(gap(1))  TypeError Traceback (most recent call last) <ipythoninput20815f7921111e> in <module> > 1 x(libgap(Integer(1))) ~/src/sagemath/sage/local/lib/python3.6/sitepackages/sage/rings/polynomial/polynomial_integer_dense_flint.pyx in sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint.__call__ (build/cythonized/sage/rings/polynomial/polynomial_integer_dense_flint.cpp:7957)() 463 return acb_z 464 > 465 return Polynomial.__call__(self, *x, **kwds) 466 467 cpdef Integer content(self): ~/src/sagemath/sage/local/lib/python3.6/sitepackages/sage/rings/polynomial/polynomial_element.pyx in sage.rings.polynomial.polynomial_element.Polynomial.__call__ (build/cythonized/sage/rings/polynomial/polynomial_element.c:9488)() 775 # polynomial's base ring (e.g., for evaluations at integers). 776 if not have_same_parent(a, cst): > 777 cst, aa = coercion_model.canonical_coercion(cst, a) 778 # Use fast multiplication actions like matrix × scalar. 779 # If there is no action, replace a by an element of the ~/src/sagemath/sage/local/lib/python3.6/sitepackages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel.canonical_coercion (build/cythonized/sage/structure/coerce.c:13758)() 1393 self._record_exception() 1394 > 1395 raise TypeError("no common canonical parent for objects with parents: '%s' and '%s'"%(xp, yp)) 1396 1397 TypeError: no common canonical parent for objects with parents: 'Integer Ring' and '<class 'gappy.gapobj.GapInteger'>'
This doesn't work with PARI gens either unless you convert the polynomial to PARI explicitly first.
comment:43 Changed 22 months ago by
TIL: The coercion model does in fact already support a _sage_
magic method, though it wasn't so easy for me to find: https://doc.sagemath.org/html/en/reference/coercion/sage/structure/coerce.html?highlight=_sage_#sage.structure.coerce.CoercionModel.canonical_coercion
Adding this to GapObj
makes it work:
sage: one = libgap(1) sage: one._sage_() 1 sage: R.<x> = PolynomialRing(ZZ) sage: x = R.gen() sage: x(one) 1 sage: a, b = coercion_model.canonical_coercion(one, x) sage: type(a) <class 'sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint'> sage: type(b) <class 'sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint'>
I wonder why pari_gen doesn't support this too.
comment:44 Changed 22 months ago by
Hrm, the problem with the above example though is that with GapElement
the result of evaluating a polynomial on a GapElement
is another GapElement
, whereas here it just converts it to the appropriate Sage type and evaluates the polynomial on that.
comment:45 Changed 22 months ago by
Commit:  c9160c10e033565c1e2b538cb55f89f4c518ae72 → 0ae791e6a1335d065d28448a80d072c26c257747 

Branch pushed to git repo; I updated commit sha1. New commits:
0ae791e  Update gappy to v0.1.3a3 with the requisite changes needed to adapt to

comment:46 Changed 22 months ago by
Commit:  0ae791e6a1335d065d28448a80d072c26c257747 → 82e7e56cdea9b8d24a3465f9661189288f54cc26 

Branch pushed to git repo; I updated commit sha1. New commits:
82e7e56  A few minor test fixes:

comment:47 Changed 22 months ago by
Description:  modified (diff) 

comment:48 Changed 22 months ago by
Milestone:  sage9.4 → sageduplicate/invalid/wontfix 

comment:49 Changed 21 months ago by
Reviewers:  → Dima Pasechnik 

Status:  needs_review → positive_review 
let's do this the other way: #31404
comment:50 Changed 21 months ago by
Resolution:  → wontfix 

Status:  positive_review → closed 
New commits:
Add gappy as a standard SPKG and dependency of sagelib.