Opened 4 years ago

Closed 2 years ago

#20204 closed defect (fixed)

Fix problems with constructing or converting to SymPy expressions

Reported by: rws Owned by:
Priority: major Milestone: sage-8.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 slelievre)

In the Sympy-1.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 Sympy-1.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 4 years ago by rws

  • Description modified (diff)

comment:2 Changed 3 years ago by rws

  • Description modified (diff)

comment:3 Changed 2 years ago by mforets

  • Cc mforets added

comment:4 Changed 2 years ago by infinity0

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#issuecomment-308207891

BTW you do not need to rebuild Sympy to apply or unapply this patch, just edit /usr/lib/python2.7/dist-packages/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).

Last edited 2 years ago by infinity0 (previous) (diff)

comment:5 follow-up: Changed 2 years ago by mforets

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)
<ipython-input-5-9fb5fbad3003> 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)
<ipython-input-5-9fb5fbad3003> in <module>()
----> 1 fs._sage_()

/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/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 rws

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 mforets

  • Branch set to u/mforets/20204
  • Commit set to 442d13b21bd3056d744ce92a19fb9b428f1df641

New commits:

442d13bcheck for SymbolicFunction in SympyConverter

comment:8 Changed 2 years ago by git

  • Commit changed from 442d13b21bd3056d744ce92a19fb9b428f1df641 to 9f40ac9e569317cd3f9c264d05f6c18f07bebe65

Branch pushed to git repo; I updated commit sha1. New commits:

9f40ac9add sympy method to NewSymbolicFunction

comment:9 follow-up: Changed 2 years ago by 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.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by 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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 2 years ago by mforets

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 git

  • Commit changed from 9f40ac9e569317cd3f9c264d05f6c18f07bebe65 to a07b72be4003315a37d9cf9c73c14b074c7d52c5

Branch pushed to git repo; I updated commit sha1. New commits:

a07b72bremove undeffun patch

comment:13 in reply to: ↑ 11 Changed 2 years ago by rws

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://patch-diff.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 follow-up: Changed 2 years ago by mforets

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)
<ipython-input-2-7d5000a51aaa> 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 rws

Replying to mforets:

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?

Yes, conversion of SymPy objects is usually done by their member function _sage_(). That means SymPy source change.

comment:16 Changed 2 years ago by git

  • Commit changed from a07b72be4003315a37d9cf9c73c14b074c7d52c5 to c9db59735ba48630a2cecb38308ad33d6b707a04

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

9d16a70Lost package-version.txt modifications : restored
d5d6b67Corrected error on patcj version
462bd68test is not corrected at this point
daa2f2cError on sympy branch corrected with a new patch
4872425forgetten to delete gg in doctest
721eed7removed whitespace
e46345bchanged sympy patch version
46ebd72Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function
e74980bMerge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function
c9db597Merge branch 't/23496/public/23496_patch_sympy_abstact_function' into t/mforets/20204

comment:17 Changed 2 years ago by mforets

  • Authors set to Marcelo Forets, Ralph Stephan
  • Cc mmancini added
  • Dependencies set to 23496
  • Milestone changed from sage-7.1 to sage-8.1
  • Status changed from new to needs_review

Last 10 new commits:

9d16a70Lost package-version.txt modifications : restored
d5d6b67Corrected error on patcj version
462bd68test is not corrected at this point
daa2f2cError on sympy branch corrected with a new patch
4872425forgetten to delete gg in doctest
721eed7removed whitespace
e46345bchanged sympy patch version
46ebd72Merge branch 'public/23496_patch_sympy_abstact_function' of git://trac.sagemath.org/sage into public/23496_patch_sympy_abstact_function
e74980bMerge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function
c9db597Merge branch 't/23496/public/23496_patch_sympy_abstact_function' into t/mforets/20204

comment:18 Changed 2 years ago by mforets

  • Dependencies changed from 23496 to #23496

comment:19 Changed 2 years ago by mforets

  • 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 2 years ago by rws

After #24006 there is need for a rewrite which makes it all much simpler. I'll try that now.

comment:21 Changed 2 years ago by mforets

awesome. i started to read your code today, so i don't get too outdated :)

comment:22 Changed 2 years ago by rws

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 2 years ago by rws

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 2 years ago by rws

For the solution we will have to patch SymPy, not possible otherwise.

comment:25 Changed 2 years ago by rws

  • Branch changed from u/mforets/20204 to u/rws/20204

comment:26 Changed 2 years ago by git

  • Commit changed from c9db59735ba48630a2cecb38308ad33d6b707a04 to 5010acf00b2811420ba1a725deb2351f77eb0c01

Branch pushed to git repo; I updated commit sha1. New commits:

b261ec323923: fix doctest
5010acfMerge branch 'u/rws/23923' of git://trac.sagemath.org/sage into t/20204/20204

comment:27 Changed 2 years ago by rws

  • Authors changed from Marcelo Forets, Ralph Stephan to Marcelo Forets, Ralf Stephan
  • Dependencies changed from #23496 to #23496, #22566

The original examples all work now. I'll add them as doctests tomorrow.

comment:28 Changed 2 years ago by git

  • Commit changed from 5010acf00b2811420ba1a725deb2351f77eb0c01 to 14b1c523c468bfb304191edfe6e85a5c94e916f5

Branch pushed to git repo; I updated commit sha1. New commits:

14b1c5220204: more doctests

comment:29 Changed 2 years ago by rws

  • 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 2 years ago by mforets

  • Reviewers set to Marcelo Forets

comment:31 Changed 2 years ago by mforets

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 follow-up: Changed 2 years ago by rws

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 2 years ago by mforets

  • 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 2 years ago by rws

Thanks for the review.

comment:35 Changed 2 years ago by slelievre

  • Description modified (diff)

(Fixing formatting in ticket description.)

comment:36 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:37 Changed 2 years ago by git

  • Commit changed from 14b1c523c468bfb304191edfe6e85a5c94e916f5 to fd0db783a1d0dfe70a5ca5272d762345376f0965

Branch pushed to git repo; I updated commit sha1. New commits:

9169924Merge branch 'develop' into t/24006/24006
d43419f24006: bump sympy patchlevel
fd0db78Merge commit 'd43419f73f455259896cbd9aeff86bbaa5d764c7' of git://trac.sagemath.org/sage into t/20204/20204

comment:38 Changed 2 years ago by rws

  • Status changed from needs_work to positive_review

New commits:

9169924Merge branch 'develop' into t/24006/24006
d43419f24006: bump sympy patchlevel
fd0db78Merge commit 'd43419f73f455259896cbd9aeff86bbaa5d764c7' of git://trac.sagemath.org/sage into t/20204/20204

comment:39 Changed 2 years ago by vbraun

  • Branch changed from u/rws/20204 to fd0db783a1d0dfe70a5ca5272d762345376f0965
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.