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: | 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, 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) <ipython-input-101-eeb95aeda6e6> 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 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 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/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 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 cherry-pick (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 package-version.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 "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 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: | sage-8.0 → sage-8.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/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 ----------------------------------------------------------------------
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 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 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