Opened 2 years ago

Closed 23 months ago

Last modified 22 months ago

#24006 closed enhancement (fixed)

SymPy --> Sage conversion completely inside Sage

Reported by: rws Owned by:
Priority: major Milestone: sage-8.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 rws)

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

Attachments (1)

dochtml.log.gz (11.3 KB) - added by charpent 2 years ago.
Log of a failed attempt to build the documentation.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 2 years ago by rws

  • Description modified (diff)

comment:2 Changed 2 years ago by rws

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

comment:3 Changed 2 years ago by rws

  • Branch set to u/rws/experimental__sympy_____sage_conversion_completely_inside_sage

comment:4 Changed 2 years ago by rws

  • 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:

252e5cb24006: SymPy/Sage conversion

comment:5 Changed 2 years ago by rws

  • Status changed from new to needs_review

comment:6 Changed 2 years ago by charpent

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 follow-up: Changed 2 years ago by rws

  • 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 ; follow-up: Changed 2 years ago by charpent

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 rws

Replying to charpent:

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

comment:10 Changed 2 years ago by rws

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 rws

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 git

  • Commit changed from 252e5cb3ec0b8a4c16d1576d5060c73632aa1ec1 to 31e038dd31218ba48434beb93455b71c8f484b97

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

31e038d24006: fixes

comment:13 Changed 2 years ago by git

  • Commit changed from 31e038dd31218ba48434beb93455b71c8f484b97 to 6e36053c039d27d6abacfb8aefd6f5925d13295c

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

8c3c37dfixes
6e3605324006: fixes, first part of doctests

comment:14 Changed 2 years ago by rws

  • Branch changed from u/rws/experimental__sympy_____sage_conversion_completely_inside_sage to u/rws/24006

comment:15 Changed 2 years ago by rws

  • 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:

946295423990: Convert symbolic relations to SymPy
07f474423990: handle unequality
0ed659f23990: do not evaluate
5f023ea23990: convert relations from SymPy to Sage, with test
0f596bb23990: fix patch
479e20623990: sympy patchlevel bump
3e362ee24006: SymPy --> Sage conversion completely inside Sage

Changed 2 years ago by charpent

Log of a failed attempt to build the documentation.

comment:16 Changed 2 years ago by charpent

  • 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 doc-clean && make ptestlong).

==>needs_work

comment:17 Changed 2 years ago by git

  • Commit changed from 3e362ee1b623af7aa878676b20c2609c434be321 to 8dd5f88303b980b6796e07e2a7958030263cbd74

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

8dd5f8824006: add missing file, fixes

comment:18 Changed 2 years ago by rws

  • Authors set to Ralf Stephan
  • Status changed from needs_work to needs_review

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

comment:19 follow-up: Changed 2 years ago by charpent

  • 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,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

Last edited 2 years ago by charpent (previous) (diff)

comment:20 in reply to: ↑ 19 Changed 2 years ago by rws

Replying to charpent:

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

See #23923

Oh, BTW : positive_review

Thanks.

comment:21 Changed 2 years ago by mmancini

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 rws

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 mmancini

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

comment:24 follow-up: Changed 2 years ago by jdemeyer

  • 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 follow-up: Changed 2 years ago by jdemeyer

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 ; follow-up: Changed 2 years ago by rws

  • Status changed from needs_work to needs_review

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.

comment:27 in reply to: ↑ 24 ; follow-up: Changed 2 years ago by 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.

comment:28 in reply to: ↑ 27 Changed 2 years ago by jdemeyer

  • 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 ; follow-up: Changed 2 years ago by jdemeyer

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 ; follow-up: Changed 2 years ago by rws

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 git

  • Commit changed from 8dd5f88303b980b6796e07e2a7958030263cbd74 to d43419f73f455259896cbd9aeff86bbaa5d764c7

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

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

comment:32 Changed 2 years ago by rws

  • Milestone changed from sage-8.2 to sage-8.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.

Last edited 2 years ago by rws (previous) (diff)

comment:33 in reply to: ↑ 30 Changed 2 years ago by jdemeyer

Replying to rws:

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.

comment:34 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:35 Changed 2 years ago by rws

Thanks.

comment:36 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:37 Changed 2 years ago by tscrim

  • Reviewers set to Marco Mancini, Jeroen Demeyer
  • Status changed from needs_work to positive_review

comment:38 Changed 23 months ago by vbraun

  • Branch changed from u/rws/24006 to d43419f73f455259896cbd9aeff86bbaa5d764c7
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:39 follow-up: Changed 22 months ago by jdemeyer

  • 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 follow-up: Changed 22 months ago by 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 like Float_sage (with the same capitalisation as in Sympy)?

comment:41 in reply to: ↑ 39 Changed 22 months ago by rws

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 ; follow-up: Changed 22 months ago by rws

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 like Float_sage (with the same capitalisation as in Sympy)?

That's fine. Please open a ticket for what you intend to be done.

comment:43 in reply to: ↑ 42 Changed 22 months ago by jdemeyer

Replying to rws:

That's fine. Please open a ticket for what you intend to be done.

Done: #24067

Note: See TracTickets for help on using tickets.