SymPy --> Sage conversion completely inside Sage

[rwst]

There are 46 SymPy object member functions named _sage_() that comprise the SymPy-->Sage conversion facility. It turns out that automatic testing the code at the SymPy Travis never worked and is difficult to fix because of fitting Sage into Travis. Secondly, after initial work on it by SymPy authors work is now mainly motivated with Sage development where the maintenance of interdependent Sage and SymPy patches begins to be unwieldy and error-prone.

This branch contains experimental code that dynamically adds _sage()_ methods on the first SymPy conversion to the relevant objects.

See https://github.com/sympy/sympy/issues/13430

Depends on #23990

CC: @EmmanuelCharpentier

Component: interfaces

Author: Ralf Stephan

Branch: d43419f

Reviewer: Marco Mancini, Jeroen Demeyer

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

[rwst]

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 There are 46 SymPy object member functions named `_sage_()` that comprise the SymPy-->Sage conversion facility. It turns out that automatic testing the code at the SymPy Travis never worked and is difficult to fix because of fitting Sage into Travis. Secondly, after initial work on it by SymPy authors work is now mainly motivated with Sage development where the maintenance of interdependent Sage and SymPy patches begins to be unwieldy and error-prone.

 This branch contains experimental code that dynamically adds `sage()` methods on Sage startup to the relevant objects.
+
+See https://github.com/sympy/sympy/issues/13430

[rwst]

comment:2

Proof of concept:

sage: from sympy import Symbol
....: from sage.symbolic.ring import SR
....: 
....: def _sympysage_symbol(self):
....:     return SR.var(self.name)
....: 
....: Symbol.sage = _sympysage_symbol
....: 
sage: x._sympy_()
x
sage: type(_)
<class 'sympy.core.symbol.Symbol'>
sage: x._sympy_()
x
sage: _.sage()
x
sage: type(_)
<type 'sage.symbolic.expression.Expression'>

[rwst]

Branch: u/rws/experimental__sympy_____sage_conversion_completely_inside_sage

[rwst]

comment:4

This naive attempt already works in first tests. Setting review in order to get startup timings from the patchbots.

---

New commits:

252e5cb

24006: SymPy/Sage conversion

[rwst]

Commit: 252e5cb

[7822f248-ba56-45f1-ab3d-4de7482bdf9f]

comment:6

I get a ton of errors when ptesting it over 8.1.beta7+#23990. The resulting log file is too large to be uploaded (3,2 Mb gzipped, 332,2 Mb (!!!) uncompressed).

I'll try make distclean ; make ptestlong, but don't hold your breath...

Not changing status, pending second attempt to build.

[rwst]

comment:7

Don't waste CPU, I just wanted a patchbot on it, for the startup time. But I'm afraid already there is a 25% increase so this must be done lazily. I haven't debugged it at all.

[7822f248-ba56-45f1-ab3d-4de7482bdf9f]

comment:8

Replying to @rwst:

Don't waste CPU, I just wanted a patchbot on it, for the startup time. But I'm afraid already there is a 25% increase so this must be done lazily. I haven't debugged it at all.

Aaaarghhh... I did waste CPU. For some (now unscrutable) reason, I have been unable to make sage : some plot used in the documentation refuses to build.

Currently ptesting 8.1.beta7+#23990 again to check that the problemis indeed with the present patch (and reinstalling my 434 or so R packages...).

The next time you position needs_review to get the attention of a patchbot, could you leave a note to potential reviewers ?

[rwst]

comment:9

Replying to @EmmanuelCharpentier:

The next time you position needs_review to get the attention of a patchbot, could you leave a note to potential reviewers ?

Sorry I was not clear enough in comment:4

[rwst]

comment:10

Actually the code was really bad Python, one can see it is not the language I'm used to.

[rwst]

comment:11

Also, we may have to adapt to the _sage_ convention. Yes, all interfaces in src/sage/interface even Mma use it.

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

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

31e038d

24006: fixes

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

Changed commit from 252e5cb to 31e038d

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

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

8c3c37d	fixes
6e36053	24006: fixes, first part of doctests

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

Changed commit from 31e038d to 6e36053

[rwst]

Changed branch from u/rws/experimental__sympy_____sage_conversion_completely_inside_sage to u/rws/24006

[rwst]

Changed commit from 6e36053 to 3e362ee

[rwst]

Dependencies: #23990

[rwst]

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 There are 46 SymPy object member functions named `_sage_()` that comprise the SymPy-->Sage conversion facility. It turns out that automatic testing the code at the SymPy Travis never worked and is difficult to fix because of fitting Sage into Travis. Secondly, after initial work on it by SymPy authors work is now mainly motivated with Sage development where the maintenance of interdependent Sage and SymPy patches begins to be unwieldy and error-prone.

-This branch contains experimental code that dynamically adds `sage()` methods on Sage startup to the relevant objects.
+This branch contains experimental code that dynamically adds `_sage()_` methods on the first SymPy conversion to the relevant objects.

 See https://github.com/sympy/sympy/issues/13430

[rwst]

comment:15

This new branch is now ready for review. It already passes all symbolic tests. It may fail in other Sage parts where symbolics are used. Let's put the patchbots on it.

---

New commits:

9462954	23990: Convert symbolic relations to SymPy
07f4744	23990: handle unequality
0ed659f	23990: do not evaluate
5f023ea	23990: convert relations from SymPy to Sage, with test
0f596bb	23990: fix patch
479e206	23990: sympy patchlevel bump
3e362ee	24006: SymPy --> Sage conversion completely inside Sage

[7822f248-ba56-45f1-ab3d-4de7482bdf9f]

Attachment: dochtml.log.gz

Log of a failed attempt to build the documentation.

[7822f248-ba56-45f1-ab3d-4de7482bdf9f]

comment:16

On top of #24026 (which needs review ;-), I can't even build the documentation (hence no formal testing possible). See uploaded log.

And yes, I did ./sage -f sympy before ( make doc-clean && make ptestlong).

==>needs_work

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

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

8dd5f88

24006: add missing file, fixes

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

Changed commit from 3e362ee to 8dd5f88

[rwst]

Author: Ralf Stephan

[rwst]

comment:18

Apologies again, the main file was missing, and more changes.

[7822f248-ba56-45f1-ab3d-4de7482bdf9f]

comment:19

Testing on top of #24026, ptestlong gives me two errors :

----------------------------------------------------------------------
sage -t --long src/sage/libs/gap/assigned_names.py  # 1 doctest failed
sage -t --long src/sage/schemes/elliptic_curves/sha_tate.py  # 1 doctest failed
----------------------------------------------------------------------

Both are transient (i. e. both tests pass when ran standalone).

An example of possible usefulness :

sage: dgamma(y,k,theta)=y^(k-1)*e^(-y/theta)/(theta^k*gamma(k))
sage: (dgamma(y,k,theta).laplace(y,x)^p).expand().canonicalize_radical().inverse
....: _laplace(x,y)
ilt(1/((theta*x + 1)^(k*p)), x, y)
sage: sympy.inverse_laplace_transform((dgamma(y,k,theta).laplace(y,x)^p).expand(
....: ).canonicalize_radical(),x,y)._sage_()
k*p*theta^(-k*p)*y^(k*p - 1)*e^(-y/theta)*heaviside(y)/gamma(k*p + 1)

I won't be able to test further right now. I'll try to find more examples (that could be useful doctests...).

Did you translate Sympy's conditional expressions in your recent cases ?

Another thought : we might subclass sage() as a wrapper for _sage_() possibly allowing for further arguments, such as supplementary translation dictionaries, as originally intended for mathematica.sage() (but this does not seem to worl currently...). Another ticket ?

Oh, BTW : positive_review

[rwst]

comment:20

Replying to @EmmanuelCharpentier:

Did you translate Sympy's conditional expressions in your recent cases ?

See #23923

Oh, BTW : positive_review

Thanks.

[309a26b2-e8cb-4cec-ac13-3c5452d05479]

comment:21

Hello,

I am trying to extend your interface to the function Subs:

dH = Subs(Derivative(h(_xi_1, u/2 - v/2), _xi_1), (_xi_1,), (u/2 + v/2,))

where h is an abstract function.

I tried to add the function in src/interfaces/sympy.py :

def _sympysage_Subs(self):
    """
    EXAMPLES::

        sage: from sympy import Symbol
        sage: from sympy.core.singleton import S
    """

    args = self.args
    func = args[0]
    substi = [(args[1][i],args[2][i]) for i in range(len(args[1]))]

    print (substi)
    #s = args[0].subs(substi)
    return s

But I am experimenting recursion problems (for this reason s is commented). I tried to use the function Subs(...).doit() but it has no effect.

[rwst]

comment:22

On top of #20204 I get with your code (comment removed):

sage: from sympy import Subs, Derivative, Function, Symbol
sage: _xi_1 = Symbol('_xi_1')
sage: u = Symbol('u')
sage: v = Symbol('v')
sage: h = Function('h')
sage: x._sympy_()
x
sage: dH = Subs(Derivative(h(_xi_1, u/2 - v/2), _xi_1), (_xi_1,), (u/2 + v/2,))
sage: dH._sage_()
[(_xi_1, u/2 + v/2)]
Subs(Derivative(h(_xi_1, u/2 - v/2), _xi_1), (_xi_1,), (u/2 + v/2,))

Using this code however:

    args = self.args
    substi = dict([(args[1][i]._sage_(),args[2][i]._sage_()) for i in range(len(args[1]))])
    s = args[0]._sage_().subs(substi)

I get:

sage: dH._sage_()
D[0](h)(1/2*u + 1/2*v, 1/2*u - 1/2*v)

[309a26b2-e8cb-4cec-ac13-3c5452d05479]

comment:23

So, doing the conversion to sage and then the substitution it works. Fantastic. The substitutions didn't work in sympy only. Thanks

[jdemeyer]

comment:24

There should be a change in the sympy patchlevel if you change patches (including removing them!)

[jdemeyer]

comment:25

Sorry to spoil the party, but I don't think that it's good to require that the user does sympy_init() in order for Sympy -> Sage conversion to work. In my view, it would be much better to fix it upstream in sympy. Ideally, you should either fix sympy to fix the _sage_() method in sympy or to call sympy_init() whenever sympy is imported.

[rwst]

comment:26

Replying to @jdemeyer:

....or to call sympy_init() whenever sympy is imported.

So, how to do that? Note that #20204 already has positive review and it includes this ticket. I'm offering to implement your suggestion in another ticket which can be put on top of it all because there is no pressing need for it.

[rwst]

comment:27

Replying to @jdemeyer:

There should be a change in the sympy patchlevel if you change patches (including removing them!)

There is a change in patchlevel in #20204 which includes this ticket.

[jdemeyer]

comment:28

Replying to @rwst:

Replying to @jdemeyer:

There should be a change in the sympy patchlevel if you change patches (including removing them!)

There is a change in patchlevel in #20204 which includes this ticket.

That is irrelevant. This ticket removes a patch so this ticket should increase the patchlevel of sympy.

[jdemeyer]

comment:29

Replying to @rwst:

I'm offering to implement your suggestion in another ticket which can be put on top of it all because there is no pressing need for it.

Fine for me.

My suggestion was to patch sympy itself.

[rwst]

comment:30

Replying to @jdemeyer:

My suggestion was to patch sympy itself.

You said '...you should either fix sympy to fix the sage() method in sympy or to call sympy_init() whenever sympy is imported.'

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

Changed commit from 8dd5f88 to d43419f

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

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

9169924	Merge branch 'develop' into t/24006/24006
d43419f	24006: bump sympy patchlevel

[rwst]

comment:32

Okay the patchlevel is bumped here and the sympy_init call placed in sympy/__init__ (see #24067). Please review.

[jdemeyer]

comment:33

Replying to @rwst:

Replying to @jdemeyer:

My suggestion was to patch sympy itself.

You said '...you should either fix sympy to fix the sage() method in sympy or to call sympy_init() whenever sympy is imported.'

Exactly. I gave two possible solutions, both of which involve patching sympy.

[rwst]

comment:35

Thanks.

[vbraun]

comment:36

Reviewer name

[tscrim]

Reviewer: Marco Mancini, Jeroen Demeyer

[vbraun]

Changed branch from u/rws/24006 to d43419f

[jdemeyer]

Changed commit from d43419f to none

[jdemeyer]

comment:39

I just noticed this:

################################################################
#   Distributed under GNU GPL3, see www.gnu.org
################################################################

This is not acceptable for Sage, all files in the Sage library are supposed to be licensed under GPL version 2 or later.

Do you agree to change this to GPL version 2 or later?

[jdemeyer]

comment:40

Also, I find the names like _sympysage_float a bit awkward. If we want to upstream that, we should think about that. Is it OK to change to use names like Float_sage (with the same capitalisation as in Sympy)?

[rwst]

comment:41

Replying to @jdemeyer:

I just noticed this:

################################################################
#   Distributed under GNU GPL3, see www.gnu.org
################################################################

This is not acceptable for Sage, all files in the Sage library are supposed to be licensed under GPL version 2 or later. Do you agree to change this to GPL version 2 or later?

Agree. Actually I intended 2.