sympy patch for abstract function

[man74cio]

Generic function are not transformed from sympy to sage.
this patch implement the PR sympy/sympy#12826

Upstream: Fixed upstream, but not in a stable release.

CC: @tscrim @egourgoulhon @rwst

Component: symbolics

Author: Marco Mancini

Branch/Commit: bc21300

Reviewer: Travis Scrimshaw, Marcelo Forets

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

[man74cio]

Branch: public/22496_patch_sympy_abstact_function

[man74cio]

Changed branch from public/22496_patch_sympy_abstact_function to public/23496_patch_sympy_abstact_function

[sagetrac-git]

Commit: b764ebe

[sagetrac-git]

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

b764ebe

Better sympy patch taken from #12826 PR in sympy

[man74cio]

Changed dependencies from 22802 to none

[tscrim]

Changed reviewer from tscrim to Travis Scrimshaw

[tscrim]

comment:6

Is this ready for review?

[man74cio]

comment:7

Yes, pleiade

[man74cio]

comment:8

Yes, please

[tscrim]

comment:9

Is the setting this to needs_work a double-post-type error or is there an actual issue?

Also, could you ping upstream again? It would be good to know that they accepted the patch as-is.

[man74cio]

comment:10

Double error, sorry

[man74cio]

Upstream: Reported upstream. No feedback yet.

[tscrim]

comment:11

You need to also bump the patch level in package-version.txt so that it rebuilds sympy.

Did you send another message to upstream asking about the status of the patch?

[sagetrac-git]

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

9d16a70

Lost package-version.txt modifications : restored

[sagetrac-git]

Changed commit from b764ebe to 9d16a70

[sagetrac-git]

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

d5d6b67

Corrected error on patcj version

[sagetrac-git]

Changed commit from 9d16a70 to d5d6b67

[sagetrac-git]

Changed commit from d5d6b67 to 462bd68

[sagetrac-git]

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

462bd68

test is not corrected at this point

[man74cio]

comment:15

Replying to @tscrim:

You need to also bump the patch level in package-version.txt so that it rebuilds sympy.

Did you send another message to upstream asking about the status of the patch?

The referee said me that tests are still falling on travis. I asked to see the report (I have not access to travis).

[egourgoulhon]

comment:62

Replying to @tscrim:

I'm also a little more open to just fixing the test

Indeed, the test, as it is, is quite unnatural from a Sage's user point of view: the import of specific Sympy objects (in the line from sympy import Function, Symbol) and their mixing with some Sage objects (in the line f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)), with implicit conversions Sage <--> Sympy involved, without any user control on them, is not something desirable. The change proposed in comment:50 seems much more reasonable: one defines first f entirely as a Sage object and then explicitely convert it to a Sympy object before passing it to Sympy's rsolve, because there is no Sage's rsolve yet.

[egourgoulhon]

comment:63

Replying to @tscrim:

I guess what I am asking is do we want to fix the problem of comment:59 here or just push it to a later ticket?

+1 for a later ticket.

[tscrim]

comment:64

Replying to @egourgoulhon:

Replying to @tscrim:

I'm also a little more open to just fixing the test

Indeed, the test, as it is, is quite unnatural from a Sage's user point of view: the import of specific Sympy objects (in the line from sympy import Function, Symbol) and their mixing with some Sage objects (in the line f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)), with implicit conversions Sage <--> Sympy involved, without any user control on them, is not something desirable. The change proposed in comment:50 seems much more reasonable: one defines first f entirely as a Sage object and then explicitely convert it to a Sympy object before passing it to Sympy's rsolve, because there is no Sage's rsolve yet.

I don't think it is that unnatural as someone who wants to use sympy, something that we do suggest to people, would construct something in that fashion. One of the benefits of Sage are those implicit conversions so that users should not have to worry about them.

However, that discussion aside, I think that we should keep as close to the original doctest because it is in Calcul Mathématique avec Sage (even if it is out of date). So I believe we should just change the order of multiplication of the coefficients as in comment:56 and add a warning about it.

[mforets]

comment:65

hi, thank you all for the help debugging. with this sympy patch (at sympy/core/function.py and on top of #20204), it works for me:

class AppliedUndef(Function):
    """
    Base class for expressions resulting from the application of an undefined
    function.
    """
    .
    .
    .
    def _sage_(self):
        import sage.all as sage
        fname = self.func.__name__
        func = getattr(sage, fname)
        args = [arg._sage_() for arg in self.args]
        return func

do you already have the same _sage_ there?

[egourgoulhon]

comment:66

Replying to @tscrim:

I don't think it is that unnatural as someone who wants to use sympy, something that we do suggest to people, would construct something in that fashion. One of the benefits of Sage are those implicit conversions so that users should not have to worry about them.

Well, there is some degree of arbitrariness in these implicit conversions: if a is a Sage object and b a Sympy object, what a*b shall be? For the example of the French textbook as it is now, with a=3/2 and b=u(n+1), we want a*b to be a Sympy object, so that the final f is a Sympy object. But in other cases?

[mforets]

comment:67

So I believe we should just change the order of multiplication of the coefficients as in comment:56 and add a warning about it.

it seems that game was played before:

$ git show 12cdb58d7a02391224477d0d99312be8fc03f678
@@ -379,7 +379,7 @@ Sage example in ./recequadiff.tex, line 1197::

 Sage example in ./recequadiff.tex, line 1208::


-  sage: f = u(n+2)-u(n+1)*(3/2)+u(n)*(1/2)
+  sage: f = u(n+2)-(3/2)*u(n+1)+(1/2)*u(n)

this is a commit by Ralph.

[tscrim]

comment:68

@mforets comment:67 - All the more reason to just do the minimal change. IIRC, we needed to do that in order for the construction of f to not fail, but with this patch, it is okay.

@egourgoulhon comment:66 - Yes, there is currently some ambiguity because of things are done, but ultimately it shouldn't matter. Although if I really had to decide, I would say everything should go to Sympy objects because we cannot expect Sympy objects to work with our coercion system.

[mforets]

Changed commit from 1b5227d to d76caea

[mforets]

Changed branch from public/23496_patch_sympy_abstact_function to public/23496_patch_sympy_abstract_function

[mforets]

comment:69

ok, let's do it => needs review

note: this does not depend on #20204.

(@marco : sorry, i did not intend to create a new branch, just after pushing i realized the current says "abstact" instead of "abstract")

---

New commits:

018d753	Merge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function
94af74b	#23496 : (temp) revert order in rsolve recequadiff.tex test
d76caea	#23496 : add message for change in doctest

[sagetrac-git]

Changed commit from d76caea to bc21300

[sagetrac-git]

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

bc21300

#23496 : fix a `_sympy_init_` doctest

[mforets]

comment:71

We need to do another ticket?

IMHO yes, because this is a different issue; the new ticket could be about implementing rsolve in Sage, as an interface to sympy's rsolve.

yes, that would be a nice feature. we can use ticket #1291. also open is the sage interface to sympy's solve (#22322).

[man74cio]

comment:72

Replying to @mforets:

ok, let's do it => needs review

note: this does not depend on #20204.

(@marco : sorry, i did not intend to create a new branch, just after pushing i realized the current says "abstact" instead of "abstract")

No problem, thanks.

[mforets]

comment:73

the patchbot errrors seem to be unrelated to our patch, but on a GLPK update.

[tscrim]

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Marcelo Forets

[tscrim]

comment:74

Positive review. Thanks for your work on this.

[rwst]

comment:75

Thanks from me as well.

[man74cio]

comment:76

Thank you for the review

[vbraun]

Changed branch from public/23496_patch_sympy_abstract_function to bc21300