Opened 2 years ago

Closed 22 months ago

#22802 closed enhancement (fixed)

Symbolic to SymPy convertion for generic function

Reported by: mmancini Owned by:
Priority: major Milestone: sage-8.1
Component: symbolics Keywords: sympy
Cc: egourgoulhon, rws Merged in:
Authors: Marco Mancini Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 5a53e12 (Commits) Commit: 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083
Dependencies: #23496 Stopgaps:

Description

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_()

Change History (45)

comment:1 Changed 2 years ago by mmancini

  • Branch set to public/22802
  • Commit set to 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5

comment:2 Changed 2 years ago by mmancini

  • Branch changed from public/22802 to public/22802_sympy_abstact_function
  • Commit changed from 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 to 3da57124a6e7cd88ba7928c0a5bdf67603f321c6

New commits:

b494a4fAdded sympy symbolic method for manifolds.
3da5712merged with develop

comment:3 Changed 2 years ago by git

  • Commit changed from 3da57124a6e7cd88ba7928c0a5bdf67603f321c6 to 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5

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

comment:4 Changed 2 years ago by git

  • Commit changed from 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 to ff2c44d15474c3d27a3fb4ac82831d8d31f5cfe7

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

ff2c44dSmall change towards symbolic=>sympy conversion of generic functions

comment:5 Changed 2 years ago by egourgoulhon

  • Cc egourgoulhon added; Eric Gourgoulhon removed

comment:6 Changed 2 years ago by tscrim

  • Cc rws added

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

comment:7 Changed 2 years ago by mmancini

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

comment:8 Changed 2 years ago by mmancini

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

"sympy to sage conversion : Abstract function." https://github.com/sympy/sympy/pull/12826

comment:9 Changed 2 years ago by mforets

@mmancini: 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.

comment:10 Changed 2 years ago by rws

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.

comment:11 Changed 2 years ago by mmancini

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.

comment:12 Changed 2 years ago by git

  • Commit changed from ff2c44d15474c3d27a3fb4ac82831d8d31f5cfe7 to df4dc4cad47cc4fdeb5ded6f159c459407ab5937

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

df4dc4csymbolic=>sympy conversion for derivation of generic functions

comment:13 Changed 2 years ago by mmancini

  • Status changed from new to needs_review

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

comment:14 Changed 2 years ago by tscrim

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.

comment:15 Changed 2 years ago by mmancini

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

comment:16 Changed 2 years ago by tscrim

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.

comment:17 Changed 2 years ago by git

  • Commit changed from df4dc4cad47cc4fdeb5ded6f159c459407ab5937 to 6eaf35d6d6769ae053f8627e09c8c0cd42acbdf0

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

eb19598added patch for sympy
6eaf35dBetter sympy patch taken from #12826 PR in sympy

comment:18 Changed 2 years ago by mmancini

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.

comment:19 Changed 2 years ago by tscrim

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.

comment:20 Changed 2 years ago by mmancini

  • Dependencies set to 23496

Changes done as required. I hope it is ok.

comment:21 Changed 2 years ago by mmancini

  • Dependencies changed from 23496 to #23496

comment:22 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM (with manually increased my sympy patch level for testing, but that is a change that needs to be done on #23496).

comment:23 Changed 23 months ago by git

  • Commit changed from 6eaf35d6d6769ae053f8627e09c8c0cd42acbdf0 to b6fec4d1609eff56d31bf1b7f779ed05aa7503a7
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

b764ebeBetter sympy patch taken from #12826 PR in sympy
9d16a70Lost package-version.txt modifications : restored
d5d6b67Corrected error on patcj version
462bd68test is not corrected at this point
daa2f2cError on sympy branch corrected with a new patch
4872425forgetten to delete gg in doctest
b6fec4dmerge with sympy patch

comment:24 Changed 23 months ago by git

  • Commit changed from b6fec4d1609eff56d31bf1b7f779ed05aa7503a7 to 068d495bb14e3702a8bd9d598dc812c4f12542cb

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

721eed7removed whitespace
e46345bchanged sympy patch version
46ebd72Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function
068d495Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function

comment:25 Changed 23 months ago by tscrim

  • Status changed from needs_review to positive_review

Just a merge in of the latest #23496. Tests still pass, LGTM, thanks.

Last edited 23 months ago by tscrim (previous) (diff)

comment:26 Changed 22 months ago by mforets

  • Status changed from positive_review to needs_work

while this is "on-hold", could derivative be revised? the docstring has some defects right now:

  • no INPUT
  • extra spaces that are not conventional in sage library code
  • what is convertion :: ?

and why should the debug lines # print(...) be kept?

comment:27 Changed 22 months ago by git

  • Commit changed from 068d495bb14e3702a8bd9d598dc812c4f12542cb to 078532f03b0720617ba5a25b965ec5d6805eb541

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

078532fdocstring modifications

comment:28 Changed 22 months ago by mmancini

  • Branch changed from public/22802_sympy_abstact_function to public/22802_sympy_abstract_function
  • Commit 078532f03b0720617ba5a25b965ec5d6805eb541 deleted

Changed name because typo error

comment:29 Changed 22 months ago by mmancini

  • Branch changed from public/22802_sympy_abstract_function to public/22802_sympy_abstact_function
  • Commit set to 078532f03b0720617ba5a25b965ec5d6805eb541

Last 10 new commits:

721eed7removed whitespace
e46345bchanged sympy patch version
46ebd72Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function
ff2c44dSmall change towards symbolic=>sympy conversion of generic functions
df4dc4csymbolic=>sympy conversion for derivation of generic functions
eb19598added patch for sympy
6eaf35dBetter sympy patch taken from #12826 PR in sympy
b6fec4dmerge with sympy patch
068d495Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function
078532fdocstring modifications

comment:30 Changed 22 months ago by mmancini

  • Branch changed from public/22802_sympy_abstact_function to public/22802_sympy_abstract_function
  • Commit 078532f03b0720617ba5a25b965ec5d6805eb541 deleted

comment:31 Changed 22 months ago by git

  • Commit set to 078532f03b0720617ba5a25b965ec5d6805eb541

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

721eed7removed whitespace
e46345bchanged sympy patch version
46ebd72Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function
ff2c44dSmall change towards symbolic=>sympy conversion of generic functions
df4dc4csymbolic=>sympy conversion for derivation of generic functions
eb19598added patch for sympy
6eaf35dBetter sympy patch taken from #12826 PR in sympy
b6fec4dmerge with sympy patch
068d495Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function
078532fdocstring modifications

comment:32 Changed 22 months ago by mmancini

The name of the branch was corrected

comment:33 Changed 22 months ago by mmancini

  • Status changed from needs_work to needs_review

comment:34 Changed 22 months ago by mforets

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

https://git.sagemath.org/sage.git/diff?id=eedf6da36d74e1dd3cc996201a4160d101c3b0a6

and

https://git.sagemath.org/sage.git/diff?id2=037272ccbac814ddcacd7f0df60cc68d357d3fa0&id=bc21300122019b04a30dba02aaa2cd728ff7453f

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

comment:35 Changed 22 months ago by git

  • Commit changed from 078532f03b0720617ba5a25b965ec5d6805eb541 to 0be1d909ba8dda133a45e08a4470951e0935092f

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

67f7f46integrated modifications to composition. But an error remains in french_book.recequadiff tests
1b5227dError in french_book.recequadiff tests corrected
018d753Merge 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
0be1d90Merge branch 'public/23496_patch_sympy_abstract_function' into 22802_sympy_abstract_function

comment:36 Changed 22 months ago by git

  • Commit changed from 0be1d909ba8dda133a45e08a4470951e0935092f to 137d715abc54a1be4c53bcb329a91d848960a832

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

7a6cb0aMerge branch 'public/22802_sympy_abstract_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstract_function
137d715Trivial reviwer changes.

comment:37 Changed 22 months ago by tscrim

  • Milestone changed from sage-8.0 to sage-8.1
  • Status changed from needs_review to needs_work

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
----------------------------------------------------------------------
Last edited 22 months ago by tscrim (previous) (diff)

comment:38 Changed 22 months ago by git

  • Commit changed from 137d715abc54a1be4c53bcb329a91d848960a832 to 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083

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

5a53e12Problem SympyConverter.composition: returned to previous implementation

comment:39 Changed 22 months ago by mmancini

  • Status changed from needs_work to needs_review

comment:40 Changed 22 months ago by tscrim

  • Status changed from needs_review to positive_review

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

comment:41 Changed 22 months ago by mforets

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

comment:42 Changed 22 months ago by tscrim

This is already based on the branch on #23496.

comment:43 Changed 22 months ago by mforets

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. 

comment:44 Changed 22 months ago by mmancini

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

comment:45 Changed 22 months ago by vbraun

  • Branch changed from public/22802_sympy_abstract_function to 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.