Opened 3 years ago
Closed 20 months ago
#20204 closed defect (fixed)
Fix problems with constructing or converting to SymPy expressions
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  symbolics  Keywords:  
Cc:  mforets, mmancini  Merged in:  
Authors:  Marcelo Forets, Ralf Stephan  Reviewers:  Marcelo Forets 
Report Upstream:  N/A  Work issues:  
Branch:  fd0db78 (Commits)  Commit:  fd0db783a1d0dfe70a5ca5272d762345376f0965 
Dependencies:  #23496, #22566  Stopgaps: 
Description (last modified by )
In the Sympy1.0 upgrade the AppliedUndef._sage_
method is patched away to enable the important upgrade. This function was planned to resolve part of #14723 but uncovers problems with symbolics. The following doctest in french_book fails if AppliedUndef._sage_
is defined:
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
tscrim found the minimal cases:
sage: f = function('f') sage: x = var('x') sage: sympy.sympify(f(x), evaluate=False) f(x) sage: sympy.sympify(3*f(x), evaluate=False) AttributeError: 'Call' object has no attribute 'id'However, we do seem to have our own troubles of constructing SymPy objects from symbolic expressions. All of these result in errors:
sage: f._sympy_() sage: f(x)._sympy_() sage: (f(x)+3)._sympy_()
This means that Sympy1.0 probably only uncovered already existing problems in expression conversion. This ticket should remove the patch in build/pkgs/sympy
and resolve all mentioned problems.
See #20185 for all previous discussion on this.
Change History (39)
comment:1 Changed 3 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
 Description modified (diff)
comment:3 Changed 2 years ago by
 Cc mforets added
comment:4 Changed 2 years ago by
comment:5 followup: ↓ 6 Changed 2 years ago by
hi, i've experimented adding a fragment of #14723:
@@ 764,6 +764,9 @@ class SympyConverter(Converter): if f_sympy: return f_sympy(*sympy.sympify(g, evaluate=False)) else: + from sage.symbolic.function_factory import SymbolicFunction + if isinstance(ex.operator(), SymbolicFunction): + return sympy.Function(str(f))(*g, evaluate=False) raise NotImplementedError("SymPy function '%s' doesn't exist" % f)
and adding the _sympy_
method to the NewSymbolicFunction? class:
@@ 67,6 +67,10 @@ def function_factory(name, nargs=0, latex_name=None, conversions=None, """ return "'%s"%self.name() + def _sympy_(self): + from sympy import Function + return Function(self.name())
with these changes, the conversions in this ticket's description work. does this go in the good direction or not?
however, the round trip back to sage is broken:
sage: f = function('f') sage: type(f) <class 'sage.symbolic.function_factory.NewSymbolicFunction'> sage: fs = f._sympy_() sage: type(fs) <class 'sympy.core.function.UndefinedFunction'> sage: fs._sage_()  TypeError Traceback (most recent call last) <ipythoninput59fb5fbad3003> in <module>() > 1 fs._sage_() TypeError: unbound method _sage_() must be called with f instance as first argument (got nothing instead)
and with a different error for a function with arguments:
sage: f = function('f')(x) sage: type(f) <type 'sage.symbolic.expression.Expression'> sage: fs = f._sympy_() sage: type(fs) f sage: fs._sage_()  AttributeError Traceback (most recent call last) <ipythoninput59fb5fbad3003> in <module>() > 1 fs._sage_() /Users/forets/sagesrc/sage/local/lib/python2.7/sitepackages/sympy/core/function.pyc in _sage_(self) 705 import sage.all as sage 706 fname = self.func.__name__ > 707 func = getattr(sage, fname) 708 args = [arg._sage_() for arg in self.args] 709 return func(*args) AttributeError: 'module' object has no attribute 'f'
comment:6 in reply to: ↑ 5 Changed 2 years ago by
Replying to mforets:
with these changes, the conversions in this ticket's description work. does this go in the good direction or not?
Looks good.
comment:7 Changed 2 years ago by
 Branch set to u/mforets/20204
 Commit set to 442d13b21bd3056d744ce92a19fb9b428f1df641
New commits:
442d13b  check for SymbolicFunction in SympyConverter

comment:8 Changed 2 years ago by
 Commit changed from 442d13b21bd3056d744ce92a19fb9b428f1df641 to 9f40ac9e569317cd3f9c264d05f6c18f07bebe65
Branch pushed to git repo; I updated commit sha1. New commits:
9f40ac9  add sympy method to NewSymbolicFunction

comment:9 followup: ↓ 10 Changed 2 years ago by
for the remaining part, this is supposed to be solved here? (This ticket should remove the patch.. => to delete the file build/pkgs/sympy/patches/01_undeffun_sage.patch
??). i've played around, but i don't quite understand how it's supposed to work.. thanks.
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 2 years ago by
Replying to mforets:
for the remaining part, this is supposed to be solved here? (This ticket should remove the patch.. => to delete the file
build/pkgs/sympy/patches/01_undeffun_sage.patch
??). i've played around, but i don't quite understand how it's supposed to work.. thanks.
You mean how to delete a file from a git branch? use git rm
.
comment:11 in reply to: ↑ 10 ; followup: ↓ 13 Changed 2 years ago by
Replying to rws:
Replying to mforets:
for the remaining part, this is supposed to be solved here? (This ticket should remove the patch.. => to delete the file
build/pkgs/sympy/patches/01_undeffun_sage.patch
??). i've played around, but i don't quite understand how it's supposed to work.. thanks.You mean how to delete a file from a git branch? use
git rm
.
no; i didn't know what are these *.patch
files, how they are generated, how they are incorporated into sage.. Anyway, I'll git rm 01_undeffun_sage.patch
later today!
comment:12 Changed 2 years ago by
 Commit changed from 9f40ac9e569317cd3f9c264d05f6c18f07bebe65 to a07b72be4003315a37d9cf9c73c14b074c7d52c5
Branch pushed to git repo; I updated commit sha1. New commits:
a07b72b  remove undeffun patch

comment:13 in reply to: ↑ 11 Changed 2 years ago by
Replying to mforets:
no; i didn't know what are these
*.patch
files, how they are generated, how they are incorporated into sage..
You get them with any git branch by piping the output of git diff
into a file. OTOH you don't even need git diff
. For example the SymPy PR patch mentioned in #22802 can be had by downloading https://patchdiff.githubusercontent.com/raw/sympy/sympy/pull/12826.diff
, i.e. the PR link with .diff
appended. That should work with any github commit.
comment:14 followup: ↓ 15 Changed 2 years ago by
OK and those patches get automatically applied into /local/lib/...
upon rebuilding.
I've tested the patch in question. It solves one out of the two cases from comment:5 :
sage: function('f')(x)._sympy_()._sage_() # ok f(x) sage: function('f')._sympy_()._sage_() # problem  TypeError Traceback (most recent call last) <ipythoninput27d5000a51aaa> in <module>() > 1 function('f')._sympy_()._sage_() TypeError: unbound method _sage_() must be called with f instance as first argument (got nothing instead)
If we also want to transform from UndefinedFunction
back to NewSymbolicFunction
, then, that case should be addressed upstream in sympy to sage conversion : Abstract function too?
comment:15 in reply to: ↑ 14 Changed 2 years ago by
Replying to mforets:
If we also want to transform from
UndefinedFunction
back toNewSymbolicFunction
, then, that case should be addressed upstream in sympy to sage conversion : Abstract function too?
Yes, conversion of SymPy objects is usually done by their member function _sage_()
. That means SymPy source change.
comment:16 Changed 22 months ago by
 Commit changed from a07b72be4003315a37d9cf9c73c14b074c7d52c5 to c9db59735ba48630a2cecb38308ad33d6b707a04
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
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

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

e74980b  Merge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function

c9db597  Merge branch 't/23496/public/23496_patch_sympy_abstact_function' into t/mforets/20204

comment:17 Changed 22 months ago by
 Cc mmancini added
 Dependencies set to 23496
 Milestone changed from sage7.1 to sage8.1
 Status changed from new to needs_review
Last 10 new commits:
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

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

e74980b  Merge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function

c9db597  Merge branch 't/23496/public/23496_patch_sympy_abstact_function' into t/mforets/20204

comment:18 Changed 22 months ago by
 Dependencies changed from 23496 to #23496
comment:19 Changed 22 months ago by
 Status changed from needs_review to needs_work
essentially there is this problem:
sage: from sympy import Function, Symbol sage: u = Function('u') ; n = Symbol('n', integer=True); sage: type(2+u(2)) # wrong <type 'sage.symbolic.expression.Expression'> sage: type(u(2)+2) <class 'sympy.core.add.Add'>
it gets fixed with the code in this comment: https://trac.sagemath.org/ticket/23496#comment:65
do you agree?
comment:20 Changed 20 months ago by
After #24006 there is need for a rewrite which makes it all much simpler. I'll try that now.
comment:21 Changed 20 months ago by
awesome. i started to read your code today, so i don't get too outdated :)
comment:22 Changed 20 months ago by
I think the problem with Function('f')
is that it returns a class not an object, and I'm not sure if we can put a _sage_
method onto that.
comment:23 Changed 20 months ago by
To document what happens in SymPy:
When F=Function('abc')
is called the constructor of Function
uses the metaclass UndefFunction
to create a new class with parent AppliedUndef
named abc
. Calling F._sage_()
will NOT call the _sage_()
member of AppliedUndef
but tries to call abc._sage_()
which does not exist at installation time. This, however, is not a problem because we can dynamically add a _sage_()
method to abc
on creation of abc
inside UndefFunction.__new__
. So converting abstract functions works.
The problem is that now F(x)._sage_()
will call the same abc._sage_()
while previously it was referred to the superclass, i.e. AppliedUndef._sage_()
. But _sage_
as class method does not know about the specific instance and its argument x
.
comment:24 Changed 20 months ago by
For the solution we will have to patch SymPy, not possible otherwise.
comment:25 Changed 20 months ago by
 Branch changed from u/mforets/20204 to u/rws/20204
comment:26 Changed 20 months ago by
 Commit changed from c9db59735ba48630a2cecb38308ad33d6b707a04 to 5010acf00b2811420ba1a725deb2351f77eb0c01
comment:27 Changed 20 months ago by
 Dependencies changed from #23496 to #23496, #22566
The original examples all work now. I'll add them as doctests tomorrow.
comment:28 Changed 20 months ago by
 Commit changed from 5010acf00b2811420ba1a725deb2351f77eb0c01 to 14b1c523c468bfb304191edfe6e85a5c94e916f5
Branch pushed to git repo; I updated commit sha1. New commits:
14b1c52  20204: more doctests

comment:29 Changed 20 months ago by
 Status changed from needs_work to needs_review
 Summary changed from problems with constructing or converting to SymPy expressions to Fix problems with constructing or converting to SymPy expressions
Next, we'll work on the SymPy update, based on this ticket.
comment:30 Changed 20 months ago by
 Reviewers set to Marcelo Forets
comment:31 Changed 20 months ago by
looks good to me. doctests in src/sage/interfaces/sympy.py
pass and all (yes, all!) the issues in this ticket's description are solved. htmldoc is ok. let's wait that the patchbot changes the status..
the only oddity that i get (but maybe we shouldn't care?) is:
sage: from sympy import Function, Symbol sage: u = Function('u'); n = Symbol('n', integer=True) sage: type(u(2)+2) <class 'sympy.core.add.Add'> sage: type(2+u(2)) <type 'sage.symbolic.expression.Expression'>
comment:32 followup: ↓ 33 Changed 20 months ago by
I think there is nothing wrong. In the second case it's probably coercion that makes the choice, and it doesn't know that SymPy implements addition for their objects to use.
As to the patchbots they are unreliable at the moment. That segfault in matrix is curious but I can't reproduce it.
comment:33 in reply to: ↑ 32 Changed 20 months ago by
 Status changed from needs_review to positive_review
I think there is nothing wrong. In the second case it's probably coercion that makes the choice, and it doesn't know that SymPy? implements addition for their objects to use.
Ok.
since i cannot reproduce failures in my machine, let me set to positive review.. and thanks for fixing everything!!
comment:34 Changed 20 months ago by
Thanks for the review.
comment:35 Changed 20 months ago by
 Description modified (diff)
(Fixing formatting in ticket description.)
comment:36 Changed 20 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:37 Changed 20 months ago by
 Commit changed from 14b1c523c468bfb304191edfe6e85a5c94e916f5 to fd0db783a1d0dfe70a5ca5272d762345376f0965
comment:38 Changed 20 months ago by
 Status changed from needs_work to positive_review
comment:39 Changed 20 months ago by
 Branch changed from u/rws/20204 to fd0db783a1d0dfe70a5ca5272d762345376f0965
 Resolution set to fixed
 Status changed from positive_review to closed
Hi, any progress on this? We are currently NOT carrying Sage's sympy patch in Debian, so our Sage is running without this patch (and a few french book doctests break but we just ignore this).
In fact applying the patch, breaks some of sympy's own tests relating to Sage integration: https://github.com/sympy/sympy/pull/11858#issuecomment308207891
BTW you do not need to rebuild Sympy to apply or unapply this patch, just edit /usr/lib/python2.7/distpackages/sympy/core/function.py or wherever the file is installed to. Then restart sage (if you're testing Sage) or python (if you're testing sympy).