#24006 closed enhancement (fixed)
SymPy > Sage conversion completely inside Sage
Reported by:  Ralf Stephan  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  interfaces  Keywords:  
Cc:  Emmanuel Charpentier  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Marco Mancini, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  d43419f (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #23990  Stopgaps: 
Description (last modified by )
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 errorprone.
This branch contains experimental code that dynamically adds _sage()_
methods on the first SymPy conversion to the relevant objects.
Attachments (1)
Change History (44)
comment:1 Changed 5 years ago by
Description:  modified (diff) 

comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
Branch:  → u/rws/experimental__sympy_____sage_conversion_completely_inside_sage 

comment:4 Changed 5 years ago by
Commit:  → 252e5cb3ec0b8a4c16d1576d5060c73632aa1ec1 

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

comment:5 Changed 5 years ago by
Status:  new → needs_review 

comment:6 Changed 5 years ago by
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.
comment:7 followup: 8 Changed 5 years ago by
Status:  needs_review → needs_work 

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.
comment:8 followup: 9 Changed 5 years ago by
Replying to rws:
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 ?
comment:9 Changed 5 years ago by
comment:10 Changed 5 years ago by
Actually the code was really bad Python, one can see it is not the language I'm used to.
comment:11 Changed 5 years ago by
Also, we may have to adapt to the _sage_
convention. Yes, all interfaces in src/sage/interface
even Mma use it.
comment:12 Changed 5 years ago by
Commit:  252e5cb3ec0b8a4c16d1576d5060c73632aa1ec1 → 31e038dd31218ba48434beb93455b71c8f484b97 

Branch pushed to git repo; I updated commit sha1. New commits:
31e038d  24006: fixes

comment:13 Changed 5 years ago by
Commit:  31e038dd31218ba48434beb93455b71c8f484b97 → 6e36053c039d27d6abacfb8aefd6f5925d13295c 

comment:14 Changed 5 years ago by
Branch:  u/rws/experimental__sympy_____sage_conversion_completely_inside_sage → u/rws/24006 

comment:15 Changed 5 years ago by
Commit:  6e36053c039d27d6abacfb8aefd6f5925d13295c → 3e362ee1b623af7aa878676b20c2609c434be321 

Dependencies:  → #23990 
Description:  modified (diff) 
Status:  needs_work → needs_review 
Summary:  experimental: SymPy > Sage conversion completely inside Sage → SymPy > Sage conversion completely inside Sage 
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

Changed 5 years ago by
Attachment:  dochtml.log.gz added 

Log of a failed attempt to build the documentation.
comment:16 Changed 5 years ago by
Status:  needs_review → needs_work 

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 docclean && make ptestlong)
.
==>needs_work
comment:17 Changed 5 years ago by
Commit:  3e362ee1b623af7aa878676b20c2609c434be321 → 8dd5f88303b980b6796e07e2a7958030263cbd74 

Branch pushed to git repo; I updated commit sha1. New commits:
8dd5f88  24006: add missing file, fixes

comment:18 Changed 5 years ago by
Authors:  → Ralf Stephan 

Status:  needs_work → needs_review 
Apologies again, the main file was missing, and more changes.
comment:19 followup: 20 Changed 5 years ago by
Status:  needs_review → positive_review 

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^(k1)*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
comment:20 Changed 5 years ago by
comment:21 Changed 5 years ago by
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.
comment:22 Changed 5 years ago by
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)
comment:23 Changed 5 years ago by
So, doing the conversion to sage and then the substitution it works. Fantastic. The substitutions didn't work in sympy only. Thanks
comment:24 followup: 27 Changed 5 years ago by
Status:  positive_review → needs_work 

There should be a change in the sympy patchlevel if you change patches (including removing them!)
comment:25 followup: 26 Changed 5 years ago by
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.
comment:26 followup: 29 Changed 5 years ago by
Status:  needs_work → needs_review 

Replying to jdemeyer:
....or to call
sympy_init()
wheneversympy
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.
comment:27 followup: 28 Changed 5 years ago by
comment:28 Changed 5 years ago by
Status:  needs_review → needs_work 

Replying to rws:
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.
comment:29 followup: 30 Changed 5 years ago by
Replying to rws:
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.
comment:30 followup: 33 Changed 5 years ago by
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.'
comment:31 Changed 5 years ago by
Commit:  8dd5f88303b980b6796e07e2a7958030263cbd74 → d43419f73f455259896cbd9aeff86bbaa5d764c7 

comment:32 Changed 5 years ago by
Milestone:  sage8.2 → sage8.1 

Status:  needs_work → needs_review 
Okay the patchlevel is bumped here and the sympy_init
call placed in sympy/__init__
. Please review.
comment:33 Changed 5 years ago by
comment:34 Changed 5 years ago by
Status:  needs_review → positive_review 

comment:37 Changed 5 years ago by
Reviewers:  → Marco Mancini, Jeroen Demeyer 

Status:  needs_work → positive_review 
comment:38 Changed 5 years ago by
Branch:  u/rws/24006 → d43419f73f455259896cbd9aeff86bbaa5d764c7 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:39 followup: 41 Changed 5 years ago by
Commit:  d43419f73f455259896cbd9aeff86bbaa5d764c7 

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?
comment:40 followup: 42 Changed 5 years ago by
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)?
comment:41 Changed 5 years ago by
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.
comment:42 followup: 43 Changed 5 years ago by
Replying to jdemeyer:
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 likeFloat_sage
(with the same capitalisation as in Sympy)?
That's fine. Please open a ticket for what you intend to be done.
Proof of concept: