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

Priority:  major  Milestone:  sage8.1 
Component:  interfaces  Keywords:  
Cc:  charpent  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Marco Mancini, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  d43419f (Commits)  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 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
 Branch set to u/rws/experimental__sympy_____sage_conversion_completely_inside_sage
comment:4 Changed 2 years ago by
 Commit set to 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 2 years ago by
 Status changed from new to needs_review
comment:6 Changed 2 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 2 years ago by
 Status changed from needs_review to 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 in reply to: ↑ 7 ; followup: ↓ 9 Changed 2 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 in reply to: ↑ 8 Changed 2 years ago by
comment:10 Changed 2 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 2 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 2 years ago by
 Commit changed from 252e5cb3ec0b8a4c16d1576d5060c73632aa1ec1 to 31e038dd31218ba48434beb93455b71c8f484b97
Branch pushed to git repo; I updated commit sha1. New commits:
31e038d  24006: fixes

comment:13 Changed 2 years ago by
 Commit changed from 31e038dd31218ba48434beb93455b71c8f484b97 to 6e36053c039d27d6abacfb8aefd6f5925d13295c
comment:14 Changed 2 years ago by
 Branch changed from u/rws/experimental__sympy_____sage_conversion_completely_inside_sage to u/rws/24006
comment:15 Changed 2 years ago by
 Commit changed from 6e36053c039d27d6abacfb8aefd6f5925d13295c to 3e362ee1b623af7aa878676b20c2609c434be321
 Dependencies set to #23990
 Description modified (diff)
 Status changed from needs_work to needs_review
 Summary changed from experimental: SymPy > Sage conversion completely inside Sage to 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

comment:16 Changed 2 years ago by
 Status changed from needs_review to 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 2 years ago by
 Commit changed from 3e362ee1b623af7aa878676b20c2609c434be321 to 8dd5f88303b980b6796e07e2a7958030263cbd74
Branch pushed to git repo; I updated commit sha1. New commits:
8dd5f88  24006: add missing file, fixes

comment:18 Changed 2 years ago by
 Status changed from needs_work to needs_review
Apologies again, the main file was missing, and more changes.
comment:19 followup: ↓ 20 Changed 2 years ago by
 Status changed from needs_review to 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,y,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(k^((k  1)*p)*theta^(k*p)*gamma(k)^(p)*e^(k*p/theta)/x^p, x, y) sage: sympy.inverse_laplace_transform((dgamma(y,k,theta).laplace(y,x)^p).expand( ....: ).canonicalize_radical(),x,y)._sage_() k^((k  1)*p)*theta^(k*p)*y^(p  1)*(gamma(k + 1)/k)^(p)*e^(k*p/theta)*heaviside(y)/gamma(p)
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 in reply to: ↑ 19 Changed 2 years ago by
comment:21 Changed 2 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 2 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 2 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 2 years ago by
 Status changed from positive_review to needs_work
There should be a change in the sympy patchlevel if you change patches (including removing them!)
comment:25 followup: ↓ 26 Changed 2 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 in reply to: ↑ 25 ; followup: ↓ 29 Changed 2 years ago by
 Status changed from needs_work to 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 in reply to: ↑ 24 ; followup: ↓ 28 Changed 2 years ago by
comment:28 in reply to: ↑ 27 Changed 2 years ago by
 Status changed from needs_review to 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 in reply to: ↑ 26 ; followup: ↓ 30 Changed 2 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 in reply to: ↑ 29 ; followup: ↓ 33 Changed 2 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 2 years ago by
 Commit changed from 8dd5f88303b980b6796e07e2a7958030263cbd74 to d43419f73f455259896cbd9aeff86bbaa5d764c7
comment:32 Changed 2 years ago by
 Milestone changed from sage8.2 to sage8.1
 Status changed from needs_work to needs_review
Okay the patchlevel is bumped here and the sympy_init
call placed in sympy/__init__
(see #24067). Please review.
comment:33 in reply to: ↑ 30 Changed 2 years ago by
comment:34 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:35 Changed 2 years ago by
Thanks.
comment:37 Changed 2 years ago by
 Reviewers set to Marco Mancini, Jeroen Demeyer
 Status changed from needs_work to positive_review
comment:38 Changed 2 years ago by
 Branch changed from u/rws/24006 to d43419f73f455259896cbd9aeff86bbaa5d764c7
 Resolution set to fixed
 Status changed from positive_review to closed
comment:39 followup: ↓ 41 Changed 23 months ago by
 Commit d43419f73f455259896cbd9aeff86bbaa5d764c7 deleted
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 23 months 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 in reply to: ↑ 39 Changed 23 months 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 in reply to: ↑ 40 ; followup: ↓ 43 Changed 23 months 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: