Opened 6 years ago
Closed 5 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 6 years ago by
Branch:  → public/22802 

Commit:  → 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 
comment:2 Changed 6 years ago by
Branch:  public/22802 → public/22802_sympy_abstact_function 

Commit:  89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 → 3da57124a6e7cd88ba7928c0a5bdf67603f321c6 
comment:3 Changed 6 years ago by
Commit:  3da57124a6e7cd88ba7928c0a5bdf67603f321c6 → 89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 

Branch pushed to git repo; I updated commit sha1. New commits:
comment:4 Changed 6 years ago by
Commit:  89e6ef4aea6f5af63a9ae40a998413dfc0bb39b5 → 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 6 years ago by
Cc:  egourgoulhon added; Eric Gourgoulhon removed 

comment:6 Changed 6 years ago by
Cc:  rws added 

Is this ready for review (other than removing the commented out debugging code)?
comment:7 Changed 6 years ago by
No, the work is still not finished : composed functions are not coverted.
comment:8 Changed 6 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 6 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 6 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 6 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 6 years ago by
Commit:  ff2c44d15474c3d27a3fb4ac82831d8d31f5cfe7 → df4dc4cad47cc4fdeb5ded6f159c459407ab5937 

Branch pushed to git repo; I updated commit sha1. New commits:
df4dc4c  symbolic=>sympy conversion for derivation of generic functions

comment:13 Changed 6 years ago by
Status:  new → 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 6 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 6 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 6 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 6 years ago by
Commit:  df4dc4cad47cc4fdeb5ded6f159c459407ab5937 → 6eaf35d6d6769ae053f8627e09c8c0cd42acbdf0 

comment:18 Changed 6 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 6 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:21 Changed 6 years ago by
Dependencies:  23496 → #23496 

comment:22 Changed 6 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → 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 6 years ago by
Commit:  6eaf35d6d6769ae053f8627e09c8c0cd42acbdf0 → b6fec4d1609eff56d31bf1b7f779ed05aa7503a7 

Status:  positive_review → 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 6 years ago by
Commit:  b6fec4d1609eff56d31bf1b7f779ed05aa7503a7 → 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 6 years ago by
Status:  needs_review → positive_review 

Just a merge in of the latest #23496. Tests still pass, LGTM, thanks.
comment:26 Changed 5 years ago by
Status:  positive_review → 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 5 years ago by
Commit:  068d495bb14e3702a8bd9d598dc812c4f12542cb → 078532f03b0720617ba5a25b965ec5d6805eb541 

Branch pushed to git repo; I updated commit sha1. New commits:
078532f  docstring modifications

comment:28 Changed 5 years ago by
Branch:  public/22802_sympy_abstact_function → public/22802_sympy_abstract_function 

Commit:  078532f03b0720617ba5a25b965ec5d6805eb541 
Changed name because typo error
comment:29 Changed 5 years ago by
Branch:  public/22802_sympy_abstract_function → public/22802_sympy_abstact_function 

Commit:  → 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 5 years ago by
Branch:  public/22802_sympy_abstact_function → public/22802_sympy_abstract_function 

Commit:  078532f03b0720617ba5a25b965ec5d6805eb541 
comment:31 Changed 5 years ago by
Commit:  → 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:33 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:34 Changed 5 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 5 years ago by
Commit:  078532f03b0720617ba5a25b965ec5d6805eb541 → 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 5 years ago by
Commit:  0be1d909ba8dda133a45e08a4470951e0935092f → 137d715abc54a1be4c53bcb329a91d848960a832 

comment:37 Changed 5 years ago by
Milestone:  sage8.0 → sage8.1 

Status:  needs_review → 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 5 years ago by
Commit:  137d715abc54a1be4c53bcb329a91d848960a832 → 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083 

Branch pushed to git repo; I updated commit sha1. New commits:
5a53e12  Problem SympyConverter.composition: returned to previous implementation

comment:39 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:40 Changed 5 years ago by
Status:  needs_review → positive_review 

I do not get any of the errors reported by the patchbot. Positive review.
comment:41 Changed 5 years ago by
there is at least a merge conflict with its dependency, introduced in 5a53e12
comment:43 Changed 5 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 5 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 5 years ago by
Branch:  public/22802_sympy_abstract_function → 5a53e124d4bdf29e7d95fab9b2c5ae3f880a3083 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Added sympy symbolic method for manifolds.
merged with develop