Opened 3 years ago
Closed 3 years ago
#23496 closed defect (fixed)
sympy patch for abstract function
Reported by:  mmancini  Owned by:  mmancini 

Priority:  major  Milestone:  sage8.1 
Component:  symbolics  Keywords:  
Cc:  tscrim, egourgoulhon, rws  Merged in:  
Authors:  Marco Mancini  Reviewers:  Travis Scrimshaw, Marcelo Forets 
Report Upstream:  Fixed upstream, but not in a stable release.  Work issues:  
Branch:  bc21300 (Commits)  Commit:  bc21300122019b04a30dba02aaa2cd728ff7453f 
Dependencies:  Stopgaps: 
Description
Generic function are not transformed from sympy to sage. this patch implement the PR https://github.com/sympy/sympy/pull/12826
Change History (77)
comment:1 Changed 3 years ago by
 Branch set to public/22496_patch_sympy_abstact_function
comment:2 Changed 3 years ago by
 Component changed from PLEASE CHANGE to symbolics
 Owner changed from (none) to mmancini
comment:3 Changed 3 years ago by
 Branch changed from public/22496_patch_sympy_abstact_function to public/23496_patch_sympy_abstact_function
comment:4 Changed 3 years ago by
 Commit set to b764ebe0dbe0fcf4f2758ca4e87bebff9116db94
comment:5 Changed 3 years ago by
 Dependencies 22802 deleted
comment:6 Changed 3 years ago by
 Cc tscrim added
 Reviewers changed from tscrim to Travis Scrimshaw
Is this ready for review?
comment:9 Changed 3 years ago by
Is the setting this to needs_work a doubleposttype error or is there an actual issue?
Also, could you ping upstream again? It would be good to know that they accepted the patch asis.
comment:10 Changed 3 years ago by
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
 Status changed from needs_work to needs_review
Double error, sorry
comment:11 followup: ↓ 15 Changed 3 years ago by
You need to also bump the patch level in packageversion.txt
so that it rebuilds sympy.
Did you send another message to upstream asking about the status of the patch?
comment:12 Changed 3 years ago by
 Commit changed from b764ebe0dbe0fcf4f2758ca4e87bebff9116db94 to 9d16a70b2daf9e123f2bf180547f38a4a10c2306
Branch pushed to git repo; I updated commit sha1. New commits:
9d16a70  Lost packageversion.txt modifications : restored

comment:13 Changed 3 years ago by
 Commit changed from 9d16a70b2daf9e123f2bf180547f38a4a10c2306 to d5d6b6733ea408b4a73860578e8b482d6dfea2c7
Branch pushed to git repo; I updated commit sha1. New commits:
d5d6b67  Corrected error on patcj version

comment:14 Changed 3 years ago by
 Commit changed from d5d6b6733ea408b4a73860578e8b482d6dfea2c7 to 462bd681d32ee87a3813caa79e43842a4477d2e7
Branch pushed to git repo; I updated commit sha1. New commits:
462bd68  test is not corrected at this point

comment:15 in reply to: ↑ 11 Changed 3 years ago by
Replying to tscrim:
You need to also bump the patch level in
packageversion.txt
so that it rebuilds sympy.Did you send another message to upstream asking about the status of the patch?
The referee said me that tests are still falling on travis. I asked to see the report (I have not access to travis).
comment:16 Changed 3 years ago by
comment:17 Changed 3 years ago by
I have submitted the correction to the sympy ticket, If it passes the tests then I will change it also in the current branch
comment:18 Changed 3 years ago by
 Commit changed from 462bd681d32ee87a3813caa79e43842a4477d2e7 to daa2f2cc0a70480553db6a01fd9b28552e63920e
Branch pushed to git repo; I updated commit sha1. New commits:
daa2f2c  Error on sympy branch corrected with a new patch

comment:19 Changed 3 years ago by
 Commit changed from daa2f2cc0a70480553db6a01fd9b28552e63920e to 487242514e520b0c73e24628ac2269df7cd2c0c0
Branch pushed to git repo; I updated commit sha1. New commits:
4872425  forgetten to delete gg in doctest

comment:20 Changed 3 years ago by
The sympy patch was merged 12826
comment:21 Changed 3 years ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
Just put your real names in the author field, bump the patch level in sympy, and remove the trivial whitespace change in function.pyx
. Once done, you can set a positive review on my behalf.
comment:22 Changed 3 years ago by
 Commit changed from 487242514e520b0c73e24628ac2269df7cd2c0c0 to 721eed7a72e7d86189fa6200bed75ef1c11af4a3
Branch pushed to git repo; I updated commit sha1. New commits:
721eed7  removed whitespace

comment:23 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:24 Changed 3 years ago by
I hope it is ok : the function.pyx is now at the old state, i reversed it to before my modifications.
comment:25 Changed 3 years ago by
 Milestone changed from sage8.0 to sage8.1
 Status changed from positive_review to needs_work
You still need to bump the patch level:
1.0.p2 +1.0.p3
in $SAGE_ROOT/build/pkgs/sympy/packageversion.txt
.
comment:26 Changed 3 years ago by
 Commit changed from 721eed7a72e7d86189fa6200bed75ef1c11af4a3 to e46345b0c23138d017ce6f50f44d144179c9c668
Branch pushed to git repo; I updated commit sha1. New commits:
e46345b  changed sympy patch version

comment:27 Changed 3 years ago by
 Commit changed from e46345b0c23138d017ce6f50f44d144179c9c668 to 46ebd728aac486a5dd675f5d228d90001c421635
Branch pushed to git repo; I updated commit sha1. New commits:
46ebd72  Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function

comment:28 Changed 3 years ago by
 Status changed from needs_work to positive_review
Trivial rebase with 8.1beta0
. Thanks, LGTM.
comment:30 Changed 3 years ago by
This is quite interesting as failure
sage t long /usr/lib64/python2.7/sitepackages/sage/tests/french_book/recequadiff.py too many failed tests, not using stored timings Running doctests with ID 2017080111172110bca036. Using optional=optional,sage Doctesting 1 file. sage t long /usr/lib64/python2.7/sitepackages/sage/tests/french_book/recequadiff.py ********************************************************************** File "/usr/lib64/python2.7/sitepackages/sage/tests/french_book/recequadiff.py", line 383, in sage.tests.french_book.recequadiff Failed example: f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n) Exception raised: Traceback (most recent call last): File "/usr/lib64/python2.7/sitepackages/sage/doctest/forker.py", line 512, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/lib64/python2.7/sitepackages/sage/doctest/forker.py", line 875, in compile_and_execute exec(compiled, globs) File "<doctest sage.tests.french_book.recequadiff[108]>", line 1, in <module> f = u(n+Integer(2))(Integer(3)/Integer(2))*u(n+Integer(1))+(Integer(1)/Integer(2))*u(n) File "/usr/lib64/python2.7/sitepackages/sympy/core/decorators.py", line 76, in __sympifyit_wrapper b = sympify(b, strict=True) File "/usr/lib64/python2.7/sitepackages/sympy/core/sympify.py", line 265, in sympify return a._sympy_() File "sage/symbolic/expression.pyx", line 1445, in sage.symbolic.expression.Expression._sympy_ (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/expression.cpp:11783) return sympy(self) File "/usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.py", line 218, in __call__ return self.arithmetic(ex, operator) File "/usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.py", line 716, in arithmetic ops = [sympy.sympify(self(a), evaluate=False) for a in ex.operands()] File "/usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.py", line 226, in __call__ return self.composition(ex, operator) File "/usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.py", line 767, in composition raise NotImplementedError("SymPy function '%s' doesn't exist" % f) NotImplementedError: SymPy function 'u' doesn't exist ********************************************************************** File "/usr/lib64/python2.7/sitepackages/sage/tests/french_book/recequadiff.py", line 388, in sage.tests.french_book.recequadiff Failed example: rsolve(f, u(n), {u(0):1,u(1):1}) Exception raised: Traceback (most recent call last): File "/usr/lib64/python2.7/sitepackages/sage/doctest/forker.py", line 512, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/lib64/python2.7/sitepackages/sage/doctest/forker.py", line 875, in compile_and_execute exec(compiled, globs) File "<doctest sage.tests.french_book.recequadiff[110]>", line 1, in <module> rsolve(f, u(n), {u(Integer(0)):Integer(1),u(Integer(1)):Integer(1)}) File "/usr/lib64/python2.7/sitepackages/sympy/solvers/recurr.py", line 722, in rsolve f = f.expand().collect(y.func(Wild('m', integer=True))) File "sage/symbolic/expression.pyx", line 7163, in sage.symbolic.expression.Expression.collect (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/expression.cpp:42956) cdef Expression s0 = self.coerce_in(s) File "sage/symbolic/expression.pyx", line 2956, in sage.symbolic.expression.Expression.coerce_in (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/expression.cpp:22665) return self._parent._coerce_(z) File "sage/structure/parent_old.pyx", line 230, in sage.structure.parent_old.Parent._coerce_ (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/parent_old.c:4888) return self.coerce(x) File "sage/structure/parent.pyx", line 1168, in sage.structure.parent.Parent.coerce (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/parent.c:11000) mor = self._internal_coerce_map_from(parent(x)) File "sage/structure/parent.pyx", line 2107, in sage.structure.parent.Parent._internal_coerce_map_from (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/parent.c:18735) mor = self.discover_coerce_map_from(S) File "sage/structure/parent.pyx", line 2246, in sage.structure.parent.Parent.discover_coerce_map_from (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/parent.c:19211) user_provided_mor = self._coerce_map_from_(S) File "sage/symbolic/ring.pyx", line 91, in sage.symbolic.ring.SymbolicRing._coerce_map_from_ (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/ring.cpp:4056) cpdef _coerce_map_from_(self, R): File "/usr/lib64/python2.7/sitepackages/sage/symbolic/callable.py", line 310, in _coerce_map_from_ return SymbolicRing._coerce_map_from_(self, R) File "sage/symbolic/ring.pyx", line 91, in sage.symbolic.ring.SymbolicRing._coerce_map_from_ (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/ring.cpp:5643) cpdef _coerce_map_from_(self, R): File "sage/symbolic/ring.pyx", line 167, in sage.symbolic.ring.SymbolicRing._coerce_map_from_ (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/ring.cpp:4315) if 'sympy' in R.__module__: TypeError: argument of type 'NoneType' is not iterable ********************************************************************** 1 item had failures: 2 of 115 in sage.tests.french_book.recequadiff [114 tests, 2 failures, 4.43 s]  sage t long /usr/lib64/python2.7/sitepackages/sage/tests/french_book/recequadiff.py # 2 doctests failed
The first doctest goes
Python 2.7.12 (default, Feb 16 2017, 16:28:54) [GCC 4.9.4] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> from sympy import Function, Symbol >>> u = Function('u'); n = Symbol('n', integer=True) >>> f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n) >>>
and is no problem in python. But in sage "boom". But there is more
sage: from sympy import Function, Symbol sage: u = Function('u'); n = Symbol('n', integer=True) sage: f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n)  NotImplementedError Traceback (most recent call last) <ipythoninput3a0e08a1755b9> in <module>() > 1 f = u(n+Integer(2))(Integer(3)/Integer(2))*u(n+Integer(1))+(Integer(1)/Integer(2))*u(n) /usr/lib64/python2.7/sitepackages/sympy/core/decorators.pyc in __sympifyit_wrapper(a, b) 74 # with sympy objects. Otherwise, it must be converted. 75 if not hasattr(b, '_op_priority'): > 76 b = sympify(b, strict=True) 77 return func(a, b) 78 except SympifyError: /usr/lib64/python2.7/sitepackages/sympy/core/sympify.pyc in sympify(a, locals, convert_xor, strict, rational, evaluate) 263 264 try: > 265 return a._sympy_() 266 except AttributeError: 267 pass /usr/lib64/python2.7/sitepackages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._sympy_ (/dev/shm/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/symbolic/expression.cpp:11783)() 1443 """ 1444 from sage.symbolic.expression_conversions import sympy > 1445 return sympy(self) 1446 1447 def _algebraic_(self, field): /usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.pyc in __call__(self, ex) 216 div = self.get_fake_div(ex) 217 return self.arithmetic(div, div.operator()) > 218 return self.arithmetic(ex, operator) 219 elif operator in relation_operators: 220 return self.relation(ex, operator) /usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.pyc in arithmetic(self, ex, operator) 714 import sympy 715 operator = arithmetic_operators[operator] > 716 ops = [sympy.sympify(self(a), evaluate=False) for a in ex.operands()] 717 if operator == "+": 718 return sympy.Add(*ops) /usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.pyc in __call__(self, ex) 224 return self.tuple(ex) 225 else: > 226 return self.composition(ex, operator) 227 228 def get_fake_div(self, ex): /usr/lib64/python2.7/sitepackages/sage/symbolic/expression_conversions.pyc in composition(self, ex, operator) 765 return f_sympy(*sympy.sympify(g, evaluate=False)) 766 else: > 767 raise NotImplementedError("SymPy function '%s' doesn't exist" % f) 768 769 sympy = SympyConverter() NotImplementedError: SymPy function 'u' doesn't exist sage: f = 1*u(n+2)(3/2)*u(n+1)+(1/2)*u(n) sage:
That's right, multiply the first term by 1
the problem disappear.
comment:31 Changed 3 years ago by
What is sage "boom" ?
the problem was not present before the rebase with 8.1beta0
, is it true?
comment:32 Changed 3 years ago by
Pardon me my antics. "boom" as in sage breaks down. Just a minor explosion since it doesn't crash, just tells you off. I don't know if it was present before 8.1beta0
. My only suggestion on a possible cause is #23413 but that's just a stab in the dark.
comment:33 Changed 3 years ago by
Effectively the error disappear when I revert the changes in sympy.
but it before the 8.1beta0
it was ok.
comment:34 followup: ↓ 35 Changed 3 years ago by
the missing conversion is the composition of functions in the sympy converter, which was done here: #20204.
comment:35 in reply to: ↑ 34 ; followup: ↓ 37 Changed 3 years ago by
Replying to mforets:
the missing conversion is the composition of functions in the sympy converter, which was done here: #20204.
Shall the patch be added here, or you prefer to create a dependency for this one?
(FYI: the problem with #20204 is that i couldn't patch sympy such that f._sage_()
works, so that we have conversions in both ways, as commented in the sympy PR.)
comment:36 Changed 3 years ago by
 Cc egourgoulhon added
comment:37 in reply to: ↑ 35 Changed 3 years ago by
I think that you should create a dependency on this ticket in your (#20204). This ticket was created to separate the sympy patch from the modifications done in Sage. This ticket is yet a dependency of #22802
Replying to mforets:
Replying to mforets:
the missing conversion is the composition of functions in the sympy converter, which was done here: #20204.
Shall the patch be added here, or you prefer to create a dependency for this one?
(FYI: the problem with #20204 is that i couldn't patch sympy such that
f._sage_()
works, so that we have conversions in both ways, as commented in the sympy PR.)
comment:38 Changed 3 years ago by
 Status changed from needs_work to needs_info
comment:39 followup: ↓ 40 Changed 3 years ago by
The boom was caused by the rebase with 8.10beta0
. has your branch done this operation? I think no. Before the rebase there was not error in the tests.
comment:40 in reply to: ↑ 39 Changed 3 years ago by
Replying to mmancini:
The boom was caused by the rebase with
8.10beta0
. has your branch done this operation?
the branch t/mforets/20204
has been merged with 8.1.beta3.
comment:41 Changed 3 years ago by
If you merged your stuff with #20204 and it cannot work without the rest of #20204 we have two solutions:
 make the tickets depend on each other. Feels circular but it is an indication that the tickets need to be merged together.
 mark this ticket as a duplicate and close it, leaving only the branch at #20204.
Bonus question: Will we be able to upgrade sympy to 1.1.1 easily once everything is in? Currently it leads to failures, again because of missing converter if I am not mistaken (this is a report from sageongentoo don't look for an upgrade ticket at this stage).
comment:42 Changed 3 years ago by
comment:43 Changed 3 years ago by
comment:44 Changed 3 years ago by
 Commit changed from 46ebd728aac486a5dd675f5d228d90001c421635 to 67f7f4693f271e068e0e073b70909d5013608cf0
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

comment:45 Changed 3 years ago by
 Commit changed from 67f7f4693f271e068e0e073b70909d5013608cf0 to 1b5227d6a85a540663aa138e8304a9055ba110fd
Branch pushed to git repo; I updated commit sha1. New commits:
1b5227d  Error in french_book.recequadiff tests corrected

comment:46 followup: ↓ 47 Changed 3 years ago by
@mmancini : i don't understand the last two commits. those changes are changing the purpose of this ticket. what do you think about comment:43?
comment:47 in reply to: ↑ 46 Changed 3 years ago by
Replying to mforets:
@mmancini : i don't understand the last two commits. those changes are changing the purpose of this ticket. what do you think about comment:43?
After a week of debugging I think that this is the unique to pass the tests in french_book.recequadiff : in test : 383 sage: f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n) the integer 3/2 (Sage Integer) makes necessary the modifications (you suggested) to composition function: u(n+2) is sympy (3/2)*u(n+1) is sage ... ...
the error in 388 sage: rsolve(f, u(n), {u(0):1,u(1):1}) is caused by the fact that Sympy integer are not imported.
Before the sympy patch there was not error but for me it was not correct: because the expressions mixed sympy (u,n) and sage quantities (1/2).
comment:48 Changed 3 years ago by
That is quite a piece of debugging. However, there should be some natural place to put in this import statement within the code, either in Sage or Sympy. Do you have an idea about where it should go?
comment:49 Changed 3 years ago by
I have no idea. For me the expression is not well written: if I am using sage and I want resolve it with a sympy method: before I write the expression than I covert it before to use sympy method and finally I convert back the result. But a strange mixing for me is not sane.
comment:50 followup: ↓ 52 Changed 3 years ago by
what i find odd is that:
 using
n = Symbol('n')
in thersolve
test it works,  similarly but using
from sympy.abc import n
.
to move on, i propose to modify the test as below, and report the failing use case in a new ticket.
sage: n = var('n', domain='integer'); u = function('u') sage: f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n) sage: us = u._sympy_() ; fs = f._sympy_(); sage: from sympy import rsolve; rsolve(fs, us(n), {us(0):1,us(1):1}) 3  4*2**(n)
what do you think?
comment:51 followup: ↓ 53 Changed 3 years ago by
 also works if you cast the sage integers to symbolic expressions, the rest of the test unchanged:
f = u(n+2)SR(3/2)*u(n+1)+SR(1/2)*u(n)
comment:52 in reply to: ↑ 50 ; followup: ↓ 54 Changed 3 years ago by
I agree,It is a coherent way to do things. We need to do another ticket?
Replying to mforets:
what i find odd is that:
 using
n = Symbol('n')
in thersolve
test it works, similarly but using
from sympy.abc import n
.to move on, i propose to modify the test as below, and report the failing use case in a new ticket.
sage: n = var('n', domain='integer'); u = function('u') sage: f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n) sage: us = u._sympy_() ; fs = f._sympy_(); sage: from sympy import rsolve; rsolve(fs, us(n), {us(0):1,us(1):1}) 3  4*2**(n)what do you think?
comment:53 in reply to: ↑ 51 Changed 3 years ago by
Replying to mforets:
 also works if you cast the sage integers to symbolic expressions, the rest of the test unchanged:
f = u(n+2)SR(3/2)*u(n+1)+SR(1/2)*u(n)
It is also possible to postpone rational numbers to prevent sympytwoday intermediate conversion :f = u(n+2)u(n+1)*3/2+u(n)*1/2
comment:54 in reply to: ↑ 52 ; followup: ↓ 71 Changed 3 years ago by
Replying to mmancini:
I agree,It is a coherent way to do things.
+1
We need to do another ticket?
IMHO yes, because this is a different issue; the new ticket could be about implementing rsolve
in Sage, as an interface to sympy's rsolve
.
The doctest discussed here is used as an example on page 17 of this document: https://members.loria.fr/PZimmermann/sagebook/recequadiff.pdf , which is a chapter of the English translation of the socalled "French book" (Calcul mathématique avec Sage). The latter is currently in its final preparatory stage, cf. https://groups.google.com/forum/#!topic/sagedevel/MalKzTyvrys . One should probably ask the authors to update this example (the book is not likely to be out before Sage 8.1, where the current ticket could be effective).
comment:55 Changed 3 years ago by
 Status changed from needs_info to needs_work
I understand what changes with that import statement, every Integer becomes a Sympy Integer because the preparser replaces things like 1
with Integer(1)
. I strongly believe we should be testing behavior that mixes Sage and Sympy, as this is a very natural thing to do (in Sage), so I disagree with changing the test and also 1b5227d.
comment:56 Changed 3 years ago by
Interesting, this works on my machine:
f = u(n+2)u(n+1)*(3/2)+u(n)/2
Doing more debugging currently.
comment:57 Changed 3 years ago by
It seems to boil back down to this:
sage: type((3/2) * u(n+1)) <type 'sage.symbolic.expression.Expression'> sage: type(u(n+1) * (3/2)) <class 'sympy.core.mul.Mul'>
comment:58 Changed 3 years ago by
Actually, that might just be a symptom rather than the cause. I'm wondering if there is something wrong with doing the conversion to Sympy:
sage: f = u(n+2)sympy.sympify(3/2)*u(n+1)+sympy.sympify(1/2)*u(n) # This works sage: f = u(n+2)sympy.sympify((3/2)*u(n+1))+sympy.sympify((1/2)*u(n)) # This fails
comment:59 Changed 3 years ago by
 Cc rws added
I think this is the real problem with all of this:
sage: from sympy import Symbol sage: n = Symbol('n', integer=True) sage: sympy.sympify(1+n).args[1] == n False
The roundtrip of a sympy symbol through Sage results in a different symbol:
sage: m = Symbol('n', integer=True) sage: m == n True sage: p = Symbol('n') sage: p == n False sage: sympy.simplify(SR(n)).assumptions0 {'commutative': True} sage: n.assumptions0 {'algebraic': True, 'commutative': True, 'complex': True, 'hermitian': True, 'imaginary': False, 'integer': True, 'irrational': False, 'noninteger': False, 'rational': True, 'real': True, 'transcendental': False} sage: p is sympy.sympify(SR(n)) True
So I think we need to translate assumptions over to truly fix the problem.
comment:60 followup: ↓ 62 Changed 3 years ago by
I'm also a little more open to just fixing the test (but making sure everything says in Sympy with a big warning message in the doctest).
comment:61 followup: ↓ 63 Changed 3 years ago by
 Status changed from needs_work to needs_info
I guess what I am asking is do we want to fix the problem of comment:59 here or just push it to a later ticket?
comment:62 in reply to: ↑ 60 ; followup: ↓ 64 Changed 3 years ago by
Replying to tscrim:
I'm also a little more open to just fixing the test
Indeed, the test, as it is, is quite unnatural from a Sage's user point of view: the import of specific Sympy objects (in the line from sympy import Function, Symbol
) and their mixing with some Sage objects (in the line f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n)
), with *implicit* conversions Sage <> Sympy involved, without any user control on them, is not something desirable. The change proposed in comment:50 seems much more reasonable: one defines first f
entirely as a Sage object and then *explicitely* convert it to a Sympy object before passing it to Sympy's rsolve
, because there is no Sage's rsolve
yet.
comment:63 in reply to: ↑ 61 Changed 3 years ago by
Replying to tscrim:
I guess what I am asking is do we want to fix the problem of comment:59 here or just push it to a later ticket?
+1 for a later ticket.
comment:64 in reply to: ↑ 62 ; followup: ↓ 66 Changed 3 years ago by
Replying to egourgoulhon:
Replying to tscrim:
I'm also a little more open to just fixing the test
Indeed, the test, as it is, is quite unnatural from a Sage's user point of view: the import of specific Sympy objects (in the line
from sympy import Function, Symbol
) and their mixing with some Sage objects (in the linef = u(n+2)(3/2)*u(n+1)+(1/2)*u(n)
), with *implicit* conversions Sage <> Sympy involved, without any user control on them, is not something desirable. The change proposed in comment:50 seems much more reasonable: one defines firstf
entirely as a Sage object and then *explicitely* convert it to a Sympy object before passing it to Sympy'srsolve
, because there is no Sage'srsolve
yet.
I don't think it is that unnatural as someone who wants to use sympy, something that we do suggest to people, would construct something in that fashion. One of the benefits of Sage are those implicit conversions so that users should not have to worry about them.
However, that discussion aside, I think that we should keep as close to the original doctest because it is in Calcul Mathématique avec Sage (even if it is out of date). So I believe we should just change the order of multiplication of the coefficients as in comment:56 and add a warning about it.
comment:65 Changed 3 years ago by
hi, thank you all for the help debugging. with this sympy patch (at sympy/core/function.py and on top of #20204), it works for me:
class AppliedUndef(Function): """ Base class for expressions resulting from the application of an undefined function. """ . . . def _sage_(self): import sage.all as sage fname = self.func.__name__ func = getattr(sage, fname) args = [arg._sage_() for arg in self.args] return func
do you already have the same _sage_
there?
comment:66 in reply to: ↑ 64 Changed 3 years ago by
Replying to tscrim:
I don't think it is that unnatural as someone who wants to use sympy, something that we do suggest to people, would construct something in that fashion. One of the benefits of Sage are those implicit conversions so that users should not have to worry about them.
Well, there is some degree of arbitrariness in these implicit conversions: if a
is a Sage object and b
a Sympy object, what a*b
shall be? For the example of the French textbook as it is now, with a
=3/2
and b
=u(n+1)
, we want a*b
to be a Sympy object, so that the final f
is a Sympy object. But in other cases?
comment:67 Changed 3 years ago by
So I believe we should just change the order of multiplication of the coefficients as in comment:56 and add a warning about it.
it seems that game was played before:
$ git show 12cdb58d7a02391224477d0d99312be8fc03f678 @@ 379,7 +379,7 @@ Sage example in ./recequadiff.tex, line 1197:: Sage example in ./recequadiff.tex, line 1208::  sage: f = u(n+2)u(n+1)*(3/2)+u(n)*(1/2) + sage: f = u(n+2)(3/2)*u(n+1)+(1/2)*u(n)
this is a commit by Ralph.
comment:68 Changed 3 years ago by
@mforets comment:67  All the more reason to just do the minimal change. IIRC, we needed to do that in order for the construction of f
to not fail, but with this patch, it is okay.
@egourgoulhon comment:66  Yes, there is currently some ambiguity because of things are done, but ultimately it shouldn't matter. Although if I really had to decide, I would say everything should go to Sympy objects because we cannot expect Sympy objects to work with our coercion system.
comment:69 followup: ↓ 72 Changed 3 years ago by
 Branch changed from public/23496_patch_sympy_abstact_function to public/23496_patch_sympy_abstract_function
 Commit changed from 1b5227d6a85a540663aa138e8304a9055ba110fd to d76caea0b97af8769edf9a1064db9f7486232b1d
 Status changed from needs_info to needs_review
ok, let's do it => needs review
note: this does not depend on #20204.
(@Marco : sorry, i did not intend to create a new branch, just after pushing i realized the current says "abstact" instead of "abstract")
New commits:
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

comment:70 Changed 3 years ago by
 Commit changed from d76caea0b97af8769edf9a1064db9f7486232b1d to bc21300122019b04a30dba02aaa2cd728ff7453f
Branch pushed to git repo; I updated commit sha1. New commits:
bc21300  #23496 : fix a _sympy_init_ doctest

comment:71 in reply to: ↑ 54 Changed 3 years ago by
comment:72 in reply to: ↑ 69 Changed 3 years ago by
comment:73 Changed 3 years ago by
the patchbot errrors seem to be unrelated to our patch, but on a GLPK update.
comment:74 Changed 3 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Marcelo Forets
 Status changed from needs_review to positive_review
Positive review. Thanks for your work on this.
comment:75 Changed 3 years ago by
Thanks from me as well.
comment:76 Changed 3 years ago by
Thank you for the review
comment:77 Changed 3 years ago by
 Branch changed from public/23496_patch_sympy_abstract_function to bc21300122019b04a30dba02aaa2cd728ff7453f
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Better sympy patch taken from #12826 PR in sympy