Symbolic to SymPy convertion for generic function

[man74cio]

Implement generic function coversion from Symbolic to SymPy

sage: a = function('A')(x,y)

sage: type(A)
<class 'sage.symbolic.function_factory.NewSymbolicFunction'>

sage: type(a)
<type 'sage.symbolic.expression.Expression'>

sage: a._sympy_()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-101-eeb95aeda6e6> in <module>()
----> 1 a._sympy_()

Depends on #23496

CC: @egourgoulhon @rwst

Component: symbolics

Keywords: sympy

Author: Marco Mancini

Branch/Commit: 5a53e12

Reviewer: Travis Scrimshaw

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

[man74cio]

Branch: public/22802

[man74cio]

Commit: 89e6ef4

[man74cio]

Changed commit from 89e6ef4 to 3da5712

[man74cio]

Changed branch from public/22802 to public/22802_sympy_abstact_function

[man74cio]

New commits:

b494a4f	Added sympy symbolic method for manifolds.
3da5712	merged with develop

[sagetrac-git]

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

[sagetrac-git]

Changed commit from 3da5712 to 89e6ef4

[sagetrac-git]

Changed commit from 89e6ef4 to ff2c44d

[sagetrac-git]

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

ff2c44d

Small change towards symbolic=>sympy conversion of generic functions

[tscrim]

comment:6

Is this ready for review (other than removing the commented out debugging code)?

[man74cio]

comment:7

No, the work is still not finished : composed functions are not coverted.

[man74cio]

comment:8

The inverse transformation was submitted to sympy in the pull request:

"sympy to sage conversion : Abstract function."
sympy/sympy#12826

[mforets]

comment:9

@man74cio: Thanks for the link to that sympy's thread, this is good news! how is that patch incorporated into 'sage's sympy'?

Please note that there is i think some conflict of commit ff2c44d here, with the branch in #20204.

[rwst]

comment:10

We can do either a patch ticket (i.e., adding it to build/pkgs/sympy/patches/) or wait for SymPy to accept the PR, then do an upgrade ticket here.

[man74cio]

comment:11

The PR on sympy si passed , I added tests and resolved some problems.
Add the page will be a great thing but I don't know if it is necessary for the moment.

Concerning the conflicts with #20204 : I did not see this ticket before, but yes, some problems could arrive.

[sagetrac-git]

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

df4dc4c

symbolic=>sympy conversion for derivation of generic functions

[sagetrac-git]

Changed commit from ff2c44d to df4dc4c

[man74cio]

comment:13

The conversion of generic functions (also derivatives and composed functions) between sage and sympy works fine.
For the full operabilty changes in sympy (sympy/sympy#12826) are needed

[tscrim]

comment:14

If you need upstream changes, you will have to add the corresponding patch to our version of sympy. For better granularity, it might be better to do the patching on a separate ticket.

[man74cio]

comment:15

How can I patch the sage version of sympy it is not versioned in git. Have you some doc?

[tscrim]

comment:16

You create a patch from the git commit. I think the command is something like git diff -p <commit>, but it should be easy enough to find examples via Google. From there, you simply add the patch to the $SAGE_ROOT/build/pkgs/sympy/patches folder, bump the corresponding package-version.txt to 1.0.p3, and commit. To test, you should simply need to run make build and then test as usual.

[sagetrac-git]

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

eb19598	added patch for sympy
6eaf35d	Better sympy patch taken from #12826 PR in sympy

[sagetrac-git]

Changed commit from df4dc4c to 6eaf35d

[man74cio]

comment:18

Hello,
I don't know if the procedure is correct:
in $SYMPY

git diff -p 6929ca35a33ba7813cd6d5828d96bc339ea06cb6 > 02_undeffun_sage.patch

then

mv 02_undeffun_sage.patch $SAGE_ROOT/build/pkgs/sympy/patches/
cat 1.0.p2 > $SAGE_ROOT/build/pkgs/sympy/package-version.txt

The test are ok.
I had to modify again sympy : now if the function is known in sympy (like fresneln in calculus.py tests) but not in sage, then the AttributeError is raised from sympy and catched in sage as before.

[tscrim]

comment:19

It looks okay to me, but I would prefer the sympy patch to go on a separate ticket that this ticket will then depend on. You can just cherry-pick (and squash if desired) your last two commits into a new branch and then merge that branch into this one here.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

721eed7	removed whitespace
e46345b	changed sympy patch version
46ebd72	Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function
ff2c44d	Small change towards symbolic=>sympy conversion of generic functions
df4dc4c	symbolic=>sympy conversion for derivation of generic functions
eb19598	added patch for sympy
6eaf35d	Better sympy patch taken from #12826 PR in sympy
b6fec4d	merge with sympy patch
068d495	Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function
078532f	docstring modifications

[sagetrac-git]

Commit: 078532f

[man74cio]

comment:32

The name of the branch was corrected

[mforets]

comment:34

could you please merge again with #23496 ? i haven't checked in detail, but there seem to be differences in the composition function,

sagemath/sagetrac-mirror@eedf6da

and

sagemath/sagetrac-mirror@037272c...bc21300

of course in case of conflict we want to keep the changes introduced by #23496.

[sagetrac-git]

Changed commit from 078532f to 0be1d90

[sagetrac-git]

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

67f7f46	integrated modifications to composition. But an error remains in french_book.recequadiff tests
1b5227d	Error in french_book.recequadiff tests corrected
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
bc21300	#23496 : fix a `_sympy_init_` doctest
0be1d90	Merge branch 'public/23496_patch_sympy_abstract_function' into 22802_sympy_abstract_function

[sagetrac-git]

Changed commit from 0be1d90 to 137d715

[sagetrac-git]

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

7a6cb0a	Merge branch 'public/22802_sympy_abstract_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstract_function
137d715	Trivial reviwer changes.

[tscrim]

comment:37

I did some trivial reviewer changes, but I am getting the same two real failures as on the patchbot:

sage -t src/sage/symbolic/expression_conversions.py
**********************************************************************
File "src/sage/symbolic/expression_conversions.py", line 792, in sage.symbolic.expression_conversions.SympyConverter.derivative
Failed example:
    df_sympy = df_sage._sympy_(); df_sympy
Exception raised:
    Traceback (most recent call last):
      File "/home/travis/sage-build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/travis/sage-build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 885, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.expression_conversions.SympyConverter.derivative[4]>", line 1, in <module>
        df_sympy = df_sage._sympy_(); df_sympy
      File "sage/symbolic/expression.pyx", line 1445, in sage.symbolic.expression.Expression._sympy_ (/home/travis/sage-build/src/build/cythonized/sage/symbolic/expression.cpp:11785)
        return sympy(self)
      File "/home/travis/sage-build/local/lib/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 222, in __call__
        return self.derivative(ex, operator)
      File "/home/travis/sage-build/local/lib/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 815, in derivative
        return f_sympy.diff(*sympy_arg)
    AttributeError: 'NoneType' object has no attribute 'diff'
**********************************************************************
File "src/sage/symbolic/expression_conversions.py", line 794, in sage.symbolic.expression_conversions.SympyConverter.derivative
Failed example:
    df_sympy == f_sympy.diff(x, 2, y, 1)
Exception raised:
    Traceback (most recent call last):
      File "/home/travis/sage-build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/travis/sage-build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 885, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.expression_conversions.SympyConverter.derivative[5]>", line 1, in <module>
        df_sympy == f_sympy.diff(x, Integer(2), y, Integer(1))
    NameError: name 'df_sympy' is not defined
**********************************************************************
1 item had failures:
   2 of   7 in sage.symbolic.expression_conversions.SympyConverter.derivative
    [489 tests, 2 failures, 2.37 s]
----------------------------------------------------------------------
sage -t src/sage/symbolic/expression_conversions.py  # 2 doctests failed
----------------------------------------------------------------------

[sagetrac-git]

Changed commit from 137d715 to 5a53e12

[sagetrac-git]

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

5a53e12

Problem SympyConverter.composition: returned to previous implementation

[tscrim]

comment:40

I do not get any of the errors reported by the patchbot. Positive review.

[mforets]

comment:41

there is at least a merge conflict with its dependency, introduced in ​5a53e12

[tscrim]

comment:42

This is already based on the branch on #23496.

[mforets]

comment:43

the branches do have different code for composition (since commit ​5a53e12 came after the merge).

anyway, sorry for my confusion. i didn't realize that it will just overwrite the code in the dependency, but without merge conflict,

$ git checkout t/22802/public/22802_sympy_abstract_function
Switched to branch 't/22802/public/22802_sympy_abstract_function'
Your branch is up-to-date with 'trac/public/22802_sympy_abstract_function'.
$ git merge t/23496/public/23496_patch_sympy_abstract_function
Already up-to-date. 

[man74cio]

comment:44

I had remove the changes in composition because it caused problems in this branch and it was not influent in #23496

[vbraun]

Changed branch from public/22802_sympy_abstract_function to 5a53e12