Opened 10 months ago

Closed 8 months ago

#31297 closed enhancement (wontfix)

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

Reported by: embray Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: interfaces Keywords: gap libgap gappy
Cc: tscrim, dimpase, vdelecroix 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:

Status badges

Description (last modified by 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.

Change History (50)

comment:1 Changed 10 months ago by embray

  • Description modified (diff)

comment:2 Changed 10 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/gappy
  • Commit set to ce1a235e093aa42c0cab6a0e9a5e80e7fc92ee84
  • Description modified (diff)

New commits:

ce1a235Add gappy as a standard SPKG and dependency of sagelib.

comment:3 Changed 10 months ago by embray

  • Description modified (diff)

comment:4 Changed 10 months ago by embray

  • Description modified (diff)

comment:5 Changed 10 months ago by embray

  • Description modified (diff)

comment:6 Changed 10 months ago by git

  • Commit changed from ce1a235e093aa42c0cab6a0e9a5e80e7fc92ee84 to a8aab2adfe417087af7f1ad5393ca7a61e242613

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

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

comment:7 in reply to: ↑ description ; follow-up: Changed 10 months ago by 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

comment:8 Changed 10 months ago by git

  • Commit changed from a8aab2adfe417087af7f1ad5393ca7a61e242613 to 483931628b3a4ce7ffe9e15fb28de20b5362a757

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

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

comment:9 Changed 10 months ago by embray

  • Description modified (diff)

comment:10 in reply to: ↑ 7 Changed 10 months ago by embray

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.

comment:11 follow-up: Changed 10 months ago by embray

  • Cc tscrim added
  • Status changed from new to 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 10 months ago by embray

  • Description modified (diff)

comment:13 Changed 10 months ago by embray

  • Work issues set to release gappy 0.1.0 final and update spkg version before merging

comment:14 Changed 10 months ago by mkoeppe

  • Cc dimpase added
  • Description modified (diff)

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

comment:15 Changed 10 months ago by git

  • Commit changed from 483931628b3a4ce7ffe9e15fb28de20b5362a757 to baa0f61f86120ca2bb53e6d3a039f69239a1d0ac

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

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

comment:16 Changed 10 months ago by mkoeppe

A quick remark on install-requires: 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 follow-up: Changed 10 months ago by vdelecroix

  • Cc vdelecroix 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 in reply to: ↑ 11 ; follow-up: Changed 10 months ago by 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 E7 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 E8 (which has 696729600 elements), but that will take many minutes to complete. D9 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 in reply to: ↑ 18 Changed 10 months ago by embray

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 E7 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 10 months ago by git

  • Commit changed from baa0f61f86120ca2bb53e6d3a039f69239a1d0ac to a07f20ecee90d82914116e45b31e58b93d0e3ca1

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

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

comment:21 in reply to: ↑ 17 ; follow-ups: Changed 10 months ago by 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.

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:

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

comment:22 in reply to: ↑ 21 ; follow-up: Changed 10 months ago by 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 -> <whatever>" converters. They could have .sage() methods tacked on to them without having to make subclasses.

comment:23 in reply to: ↑ 21 ; follow-up: Changed 10 months ago by vdelecroix

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)
<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.

comment:24 in reply to: ↑ 22 ; follow-up: Changed 10 months ago by 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 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 -> <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 in reply to: ↑ 23 Changed 10 months ago by embray

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 in reply to: ↑ 24 Changed 10 months ago by embray

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 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 -> <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?

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 follow-up: Changed 10 months ago by 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.

comment:28 in reply to: ↑ 27 Changed 10 months ago by vdelecroix

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.

comment:29 follow-up: Changed 10 months ago by embray

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 10 months ago by embray

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 10 months ago by git

  • Commit changed from a07f20ecee90d82914116e45b31e58b93d0e3ca1 to c9160c10e033565c1e2b538cb55f89f4c518ae72

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

c9160c1Further rework of 45b175e43621795f890204a9cdfb30b524f961fa which

comment:32 in reply to: ↑ 29 ; follow-ups: Changed 10 months ago by 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.

comment:33 in reply to: ↑ 32 ; follow-up: Changed 10 months ago by 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 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 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 in reply to: ↑ 33 Changed 10 months ago by vdelecroix

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 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 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.

comment:35 follow-up: Changed 10 months ago by 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.

comment:36 in reply to: ↑ 35 ; follow-up: Changed 10 months ago by 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/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.

comment:37 Changed 10 months ago by mkoeppe

Thanks, you are right, of course.

comment:38 follow-up: Changed 10 months ago by mkoeppe

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

comment:39 in reply to: ↑ 38 Changed 10 months ago by tscrim

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.

comment:40 in reply to: ↑ 32 Changed 10 months ago by embray

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.

comment:41 in reply to: ↑ 36 Changed 10 months ago by embray

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/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.

No, but a plain Python subclass of it is.

comment:42 Changed 10 months ago by embray

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)
<ipython-input-20-815f7921111e> in <module>
----> 1 x(libgap(Integer(1)))

~/src/sagemath/sage/local/lib/python3.6/site-packages/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/site-packages/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/site-packages/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 10 months ago by embray

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.

Last edited 10 months ago by embray (previous) (diff)

comment:44 Changed 10 months ago by embray

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 10 months ago by git

  • Commit changed from c9160c10e033565c1e2b538cb55f89f4c518ae72 to 0ae791e6a1335d065d28448a80d072c26c257747

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

0ae791eUpdate gappy to v0.1.3a3 with the requisite changes needed to adapt to

comment:46 Changed 10 months ago by git

  • Commit changed from 0ae791e6a1335d065d28448a80d072c26c257747 to 82e7e56cdea9b8d24a3465f9661189288f54cc26

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

82e7e56A few minor test fixes:

comment:47 Changed 9 months ago by embray

  • Description modified (diff)

comment:48 Changed 9 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-duplicate/invalid/wontfix

comment:49 Changed 8 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

let's do this the other way: #31404

comment:50 Changed 8 months ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.