Opened 4 years ago

Closed 4 years ago

#20185 closed enhancement (fixed)

Upgrade to SymPy-1.0

Reported by: rws Owned by:
Priority: major Milestone: sage-7.2
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Travis Scrimshaw, Ralf Stephan, Volker Braun
Report Upstream: N/A Work issues:
Branch: 3872f81 (Commits) Commit: 3872f81c60ab0ae37d2ec64e240cce80a053edc8
Dependencies: Stopgaps:

Change History (36)

comment:1 Changed 4 years ago by rws

  • Description modified (diff)

comment:2 Changed 4 years ago by rws

  • Branch set to u/rws/upgrade_to_sympy_1_0

comment:3 follow-up: Changed 4 years ago by rws

  • Commit set to 6971607ce1b72388e57a742ab8f5f0ffb4af1c0e
$ ./sage -p mpmath
Successfully installed mpmath-0.19
$ ./sage -p sympy
building sympy
Please install the mpmath package with a version >= 0.19
Error building sympy

Any hints?


New commits:

6971607pkg version/chksum/patch removed

comment:4 in reply to: ↑ 3 Changed 4 years ago by fbissey

Replying to rws:

$ ./sage -p mpmath
Successfully installed mpmath-0.19
$ ./sage -p sympy
building sympy
Please install the mpmath package with a version >= 0.19
Error building sympy

Any hints?


New commits:

6971607pkg version/chksum/patch removed

I think I have. In the past (prior to this version) sympy's setup.py was importing sympy itself. The danger then was to import the previously installed sympy rather the new sympy. So we add this

export PYTHONPATH="."
echo "building sympy"
python -S setup.py build

The -S is the cause of your trouble, look it up. So you should remove the PYTHONPATH stuff, the -S and remove the corresponding comments in spkg-install.

comment:5 Changed 4 years ago by git

  • Commit changed from 6971607ce1b72388e57a742ab8f5f0ffb4af1c0e to 907bedc76d3289455a701051d1af6c1be64135b1

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

9cdde1320185: remove python -S stuff
907bedcMerge branch 'develop' into t/20185/upgrade_to_sympy_1_0

comment:6 follow-up: Changed 4 years ago by rws

Thanks, that was it. So there are changes that affect doctests:

sage -t --warn-long 27.4 src/sage/symbolic/integration/integral.py  # 1 doctest failed
sage -t --warn-long 27.4 src/sage/symbolic/expression.pyx  # 3 doctests failed
sage -t --warn-long 27.2 src/sage/tests/french_book/recequadiff.py  # 2 doctests failed

That's only from a partial quick scan.

comment:7 in reply to: ↑ 6 Changed 4 years ago by rws

sage -t --warn-long 27.2 src/sage/tests/french_book/recequadiff.py  # 2 doctests failed

This can be minimized to

sage: import sympy
sage: u = sympy.Function('u'); n = sympy.Symbol('n', integer=True)
sage: sympy.sympify(3*u(n), evaluate=False)
NotImplementedError: SymPy function 'u' doesn't exist

which disappears when 3*u(n) is changed to u(n)*3 or u(n)

EDIT: I will just change the order in the muls for the doctest, and open a ticket. The case is rare and can be fixed after the upgrade.

Last edited 4 years ago by rws (previous) (diff)

comment:8 Changed 4 years ago by git

  • Commit changed from 907bedc76d3289455a701051d1af6c1be64135b1 to f92f927e44e1638efb08f01f2136010d236daaef

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

f92f927fix doctests

comment:9 Changed 4 years ago by tscrim

IMO comment:7 (#20194) is a serious regression and it should be fixed with the upgrade.

comment:10 Changed 4 years ago by git

  • Commit changed from f92f927e44e1638efb08f01f2136010d236daaef to 03912520043631024635c3d6d09b86743b6c411e

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

0391252fix doctest

comment:11 Changed 4 years ago by rws

The doctest is fixed but something seems not right in the sympification of undefined Sage functions. The case in #20194 crashes Sympy's parser and I have reported in https://github.com/sympy/sympy/issues/10795

comment:12 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Status changed from new to needs_review

comment:13 Changed 4 years ago by tscrim

Do I understand you correctly that 0391252 means you do not have to change the test in the French book?

comment:14 Changed 4 years ago by tscrim

Now that my Sage has finished rebuilding, the answer to my question is no, we still have a regression.

comment:15 Changed 4 years ago by tscrim

With this branch, we have:

sage: import sympy
sage: u = sympy.Function('u'); n = sympy.Symbol('n', integer=True)
sage: x = int(3)
sage: x*u(n)
3*u(n)
sage: sympy.sympify(x*u(n))
3*u(n)
sage: sympy.sympify(x*u(n), evaluate=False)
3*u(n)

sage: x = 3._sympy_()
sage: sympy.sympify(x*u(n), evaluate=False)
3*u(n)

However, I think this is the problem:

sage: type(3*u(n))
<type 'sage.symbolic.expression.Expression'>
sage: type(u(n)*3)
<class 'sympy.core.mul.Mul'>

sage: x = int(3)
sage: type(x*u(n))
<class 'sympy.core.mul.Mul'>
sage: type(u(n)*x)
<class 'sympy.core.mul.Mul'>

So it is probably something on our end.

comment:16 Changed 4 years ago by tscrim

Yes, that is the behavior change. Contrast that with the current 7.1.rc0:

sage: import sympy
sage: u = sympy.Function('u'); n = sympy.Symbol('n', integer=True)
sage: type(3*u(n))
<class 'sympy.core.mul.Mul'>
sage: type(u(n)*3)
<class 'sympy.core.mul.Mul'>

comment:17 Changed 4 years ago by tscrim

I also get the same behavior in comment:15 when I am at commit f92f927.

comment:18 Changed 4 years ago by rws

This is SymPy?-0.7.6.1:

sage: parent(u)
<class 'sympy.core.function.UndefinedFunction'>
sage: cm = sage.structure.element.get_coercion_model()
sage: steps,res = cm.analyse(ZZ, parent(u))
sage: steps
['Will try _r_action and _l_action']

This is 1.0:

sage: parent(u)
<class 'sympy.core.function.UndefinedFunction'>
sage: cm = sage.structure.element.get_coercion_model()
sage: steps,res = cm.analyse(ZZ, parent(u))
sage: steps
['Right operand is not Sage element, will try _sage_.',
 'Will try _r_action and _l_action']

Conversion map:
  From: Integer Ring
  To:   Symbolic Ring, None))

comment:19 Changed 4 years ago by tscrim

So it seems the issue is coming from the fact that in 0.7.6.1, we have type(parent(u)) is type returning False before but with 1.0, it is True. Which, AFAICS, is only involved in discover_action, but the result is still a None.

sage: cm.discover_action(ZZ, parent(u(n)), operator.mul)   # Same in 0.7.6.1

Although to be fair, I think we should test against u(n) since parentheses get evaluated before *. In that case, I get the same result in both versions:

sage: parent(u)
<class 'sympy.core.function.UndefinedFunction'>
sage: parent(u(n))
u
sage: steps,res = cm.analyse(ZZ, parent(u(n))); steps
['Will try _r_action and _l_action']

I'm thinking we will need someone who is much more familiar with the coercion framework than I am.

comment:20 Changed 4 years ago by rws

And indeed, it was #14723 which prompted the addition of _sage_ methods to many SymPy? classes. So no way out of fixing coercion here.

comment:21 Changed 4 years ago by git

  • Commit changed from 03912520043631024635c3d6d09b86743b6c411e to 76a47e825dfc3e2cc0d56244ababdc9fb92d5481

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

12cdb58revert french book test; add sympy patch to remove UndefFunction._sage_
114733dMerge branch 'u/rws/upgrade_to_sympy_1_0' of git://trac.sagemath.org/sage into t/20185/upgrade_to_sympy_1_0
76a47e820185: revert an unnecessary change

comment:22 follow-up: Changed 4 years ago by rws

As you can see from #14723 the UndefFunction._sage_ addition only affects future changes. This branch now has this removed from SymPy?-1.0 to make it work with current Sage. Any changes to the coercion system because of future inclusion of UndefFunction._sage_ can then be dealt with in a separate ticket which I'll open shortly. I didn't do a full test on this, so I'll have to have a final look at this tomorrow.

comment:23 Changed 4 years ago by tscrim

Okay, I think I have a better understanding of things. So here's what I think is going on. The coercion system is working fine:

sage: import sympy
sage: u = sympy.Function('u'); n = sympy.Symbol('n', integer=True)
sage: import operator
sage: cm = sage.structure.element.get_coercion_model()

sage: cm.common_parent(3, u(n))
Symbolic Ring
sage: cm.common_parent(u(n), 3)
Symbolic Ring
sage: type(cm.bin_op(3, u(n), operator.mul))
<type 'sage.symbolic.expression.Expression'>
sage: type(cm.bin_op(u(n), 3, operator.mul))
<type 'sage.symbolic.expression.Expression'>

That is now returning something rather than erroring out. This is coming from

sage: (u(n))._sage_()
u(n)
sage: type(_)
<type 'sage.symbolic.expression.Expression'>

We go to the coercion framework because of __mul__ from integers. Previously, because it was erroring out, the multiplication code then tried __rmul__ from the SymPy function, which resulted in a SymPy object. Now, we get different behaviors because when the SymPy object is on the left, we call its __mul__, which results in a SymPy object.

I'm also retracting the last part of comment:15, and the issue might be in SymPy and how it tries to parse our symbolic expressions:

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_()

I'm okay with your patch. Although we typically set the first patched version to p0.

comment:24 Changed 4 years ago by tscrim

  • Branch changed from u/rws/upgrade_to_sympy_1_0 to public/packages/upgrade_sympy-20185
  • Commit changed from 76a47e825dfc3e2cc0d56244ababdc9fb92d5481 to 42dbc8986991f8be078e0e4c61a2c55eb17febda
  • Reviewers set to Travis Scrimshaw

I changed the version to p0, but this is likely to only affect you (and really, doesn't do anything). I also fixed a failing doctest in unicode_art.py as it sends things to SymPy to create teh art. Otherwise all tests pass for me.


New commits:

42dbc89Changing version to p0 and fixing doctest in unicode_art.py.

comment:25 Changed 4 years ago by rws

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Ralf Stephan

Your changes are fine. Consider them as reviewed.

comment:26 in reply to: ↑ 22 Changed 4 years ago by rws

...Any changes to the coercion system because of future inclusion of UndefFunction._sage_ can then be dealt with in a separate ticket which I'll open shortly.

See #20204.

Last edited 4 years ago by rws (previous) (diff)

comment:27 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

Thank you.

comment:28 Changed 4 years ago by rws

Thanks for the review.

comment:29 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs fail

! Emergency stop.
<inserted text> 
                $
l.22231 \PYG{g+go}{    ⎮   √x}

comment:30 Changed 4 years ago by rws

  • Status changed from needs_work to needs_info

Cannot confirm with ./sage --docbuild all pdf. This is on OpenSuSE Leap with TeXLive 201384.

comment:31 Changed 4 years ago by tscrim

  • Milestone changed from sage-7.1 to sage-7.2
  • Status changed from needs_info to positive_review

I am unable to reproduce this either on my laptop either. Volker, can you post the logs if the buildbot fails again, along with its latex setup? Otherwise what we will have to do is change the test...

comment:32 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
$ rpm -q texlive
texlive-2014-19.20140525_r34255.fc23.x86_64

comment:33 Changed 4 years ago by git

  • Commit changed from 42dbc8986991f8be078e0e4c61a2c55eb17febda to 3872f81c60ab0ae37d2ec64e240cce80a053edc8

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

3872f81Fix U+221A in pdf docs

comment:34 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

How could that have worked for you, \sqrt in text mode should be illegal in all TeX versions.

comment:35 Changed 4 years ago by tscrim

  • Reviewers changed from Travis Scrimshaw, Ralf Stephan to Travis Scrimshaw, Ralf Stephan, Volker Braun
  • Status changed from needs_review to positive_review

Hmm...strange. With your changes, everything works. Thank you for figuring out what the root of the problem is. :P

comment:36 Changed 4 years ago by vbraun

  • Branch changed from public/packages/upgrade_sympy-20185 to 3872f81c60ab0ae37d2ec64e240cce80a053edc8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.