Opened 4 years ago
Closed 4 years ago
#22802 closed enhancement (fixed)
Symbolic to SymPy convertion for generic function
Reported by:  mmancini  Owned by:  

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  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) <ipythoninput101eeb95aeda6e6> in <module>() > 1 a._sympy_()
Change History (45)
comment:1 Changed 4 years ago by
 Branch set to public/22802
 Commit set to 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5
comment:2 Changed 4 years ago by
 Branch changed from public/22802 to public/22802_sympy_abstact_function
 Commit changed from 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 to 3da57124a6e7cd88ba7928c0a5bdf67603f321c6
comment:3 Changed 4 years ago by
 Commit changed from 3da57124a6e7cd88ba7928c0a5bdf67603f321c6 to 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5
Branch pushed to git repo; I updated commit sha1. New commits:
comment:4 Changed 4 years ago by
 Commit changed from 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 to ff2c44d15474c3d27a3fb4ac82831d8d31f5cfe7
Branch pushed to git repo; I updated commit sha1. New commits:
ff2c44d  Small change towards symbolic=>sympy conversion of generic functions

comment:5 Changed 4 years ago by
 Cc egourgoulhon added; Eric Gourgoulhon removed
comment:6 Changed 4 years ago by
 Cc rws added
Is this ready for review (other than removing the commented out debugging code)?
comment:7 Changed 4 years ago by
No, the work is still not finished : composed functions are not coverted.
comment:8 Changed 4 years ago by
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 4 years ago by
@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 4 years ago by
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 4 years ago by
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 4 years ago by
 Commit changed from ff2c44d15474c3d27a3fb4ac82831d8d31f5cfe7 to df4dc4cad47cc4fdeb5ded6f159c459407ab5937
Branch pushed to git repo; I updated commit sha1. New commits:
df4dc4c  symbolic=>sympy conversion for derivation of generic functions

comment:13 Changed 4 years ago by
 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 4 years ago by
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 4 years ago by
How can I patch the sage version of sympy it is not versioned in git. Have you some doc?
comment:16 Changed 4 years ago by
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 packageversion.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 4 years ago by
 Commit changed from df4dc4cad47cc4fdeb5ded6f159c459407ab5937 to 6eaf35d6d6769ae053f8627e09c8c0cd42acbdf0
comment:18 Changed 4 years ago by
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/packageversion.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 4 years ago by
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 cherrypick (and squash if desired) your last two commits into a new branch and then merge that branch into this one here.
comment:20 Changed 4 years ago by
 Dependencies set to 23496
Changes done as required. I hope it is ok.
comment:21 Changed 4 years ago by
 Dependencies changed from 23496 to #23496
comment:22 Changed 4 years ago by
 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 4 years ago by
 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:
b764ebe  Better sympy patch taken from #12826 PR in sympy

9d16a70  Lost packageversion.txt modifications : restored

d5d6b67  Corrected error on patcj version

462bd68  test is not corrected at this point

daa2f2c  Error on sympy branch corrected with a new patch

4872425  forgetten to delete gg in doctest

b6fec4d  merge with sympy patch

comment:24 Changed 4 years ago by
 Commit changed from b6fec4d1609eff56d31bf1b7f779ed05aa7503a7 to 068d495bb14e3702a8bd9d598dc812c4f12542cb
Branch pushed to git repo; I updated commit sha1. 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

068d495  Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function

comment:25 Changed 4 years ago by
 Status changed from needs_review to positive_review
Just a merge in of the latest #23496. Tests still pass, LGTM, thanks.
comment:26 Changed 4 years ago by
 Status changed from positive_review to needs_work
while this is "onhold", 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 4 years ago by
 Commit changed from 068d495bb14e3702a8bd9d598dc812c4f12542cb to 078532f03b0720617ba5a25b965ec5d6805eb541
Branch pushed to git repo; I updated commit sha1. New commits:
078532f  docstring modifications

comment:28 Changed 4 years ago by
 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 4 years ago by
 Branch changed from public/22802_sympy_abstract_function to public/22802_sympy_abstact_function
 Commit set to 078532f03b0720617ba5a25b965ec5d6805eb541
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

comment:30 Changed 4 years ago by
 Branch changed from public/22802_sympy_abstact_function to public/22802_sympy_abstract_function
 Commit 078532f03b0720617ba5a25b965ec5d6805eb541 deleted
comment:31 Changed 4 years ago by
 Commit set to 078532f03b0720617ba5a25b965ec5d6805eb541
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

comment:32 Changed 4 years ago by
The name of the branch was corrected
comment:33 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:34 Changed 4 years ago by
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
of course in case of conflict we want to keep the changes introduced by #23496.
comment:35 Changed 4 years ago by
 Commit changed from 078532f03b0720617ba5a25b965ec5d6805eb541 to 0be1d909ba8dda133a45e08a4470951e0935092f
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

comment:36 Changed 4 years ago by
 Commit changed from 0be1d909ba8dda133a45e08a4470951e0935092f to 137d715abc54a1be4c53bcb329a91d848960a832
comment:37 Changed 4 years ago by
 Milestone changed from sage8.0 to sage8.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/sagebuild/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 515, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/travis/sagebuild/local/lib/python2.7/sitepackages/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/sagebuild/src/build/cythonized/sage/symbolic/expression.cpp:11785) return sympy(self) File "/home/travis/sagebuild/local/lib/python2.7/sitepackages/sage/symbolic/expression_conversions.py", line 222, in __call__ return self.derivative(ex, operator) File "/home/travis/sagebuild/local/lib/python2.7/sitepackages/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/sagebuild/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 515, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/travis/sagebuild/local/lib/python2.7/sitepackages/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 
comment:38 Changed 4 years ago by
 Commit changed from 137d715abc54a1be4c53bcb329a91d848960a832 to 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083
Branch pushed to git repo; I updated commit sha1. New commits:
5a53e12  Problem SympyConverter.composition: returned to previous implementation

comment:39 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:40 Changed 4 years ago by
 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 4 years ago by
there is at least a merge conflict with its dependency, introduced in 5a53e12
comment:42 Changed 4 years ago by
This is already based on the branch on #23496.
comment:43 Changed 4 years ago by
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 uptodate with 'trac/public/22802_sympy_abstract_function'. $ git merge t/23496/public/23496_patch_sympy_abstract_function Already uptodate.
comment:44 Changed 4 years ago by
I had remove the changes in composition because it caused problems in this branch and it was not influent in #23496
comment:45 Changed 4 years ago by
 Branch changed from public/22802_sympy_abstract_function to 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Added sympy symbolic method for manifolds.
merged with develop