Opened 5 years ago
Closed 5 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: |
Description (last modified by )
https://github.com/sympy/sympy/wiki/Release-Notes-for-1.0
It could be installed as usually
https://github.com/sympy/sympy/releases/download/sympy-1.0/sympy-1.0.tar.gz
or via pip install -U sympy
.
Change History (36)
comment:1 Changed 5 years ago by
- Description modified (diff)
comment:2 Changed 5 years ago by
- Branch set to u/rws/upgrade_to_sympy_1_0
comment:3 follow-up: ↓ 4 Changed 5 years ago by
- Commit set to 6971607ce1b72388e57a742ab8f5f0ffb4af1c0e
comment:4 in reply to: ↑ 3 Changed 5 years ago by
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 sympyAny hints?
New commits:
6971607 pkg 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 5 years ago by
- Commit changed from 6971607ce1b72388e57a742ab8f5f0ffb4af1c0e to 907bedc76d3289455a701051d1af6c1be64135b1
comment:6 follow-up: ↓ 7 Changed 5 years ago by
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 5 years ago by
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.
comment:8 Changed 5 years ago by
- Commit changed from 907bedc76d3289455a701051d1af6c1be64135b1 to f92f927e44e1638efb08f01f2136010d236daaef
Branch pushed to git repo; I updated commit sha1. New commits:
f92f927 | fix doctests
|
comment:9 Changed 5 years ago by
comment:10 Changed 5 years ago by
- Commit changed from f92f927e44e1638efb08f01f2136010d236daaef to 03912520043631024635c3d6d09b86743b6c411e
Branch pushed to git repo; I updated commit sha1. New commits:
0391252 | fix doctest
|
comment:11 Changed 5 years ago by
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 5 years ago by
- Status changed from new to needs_review
comment:13 Changed 5 years ago by
Do I understand you correctly that 0391252 means you do not have to change the test in the French book?
comment:14 Changed 5 years ago by
Now that my Sage has finished rebuilding, the answer to my question is no, we still have a regression.
comment:15 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
I also get the same behavior in comment:15 when I am at commit f92f927.
comment:18 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
comment:21 Changed 5 years ago by
- Commit changed from 03912520043631024635c3d6d09b86743b6c411e to 76a47e825dfc3e2cc0d56244ababdc9fb92d5481
Branch pushed to git repo; I updated commit sha1. New commits:
12cdb58 | revert french book test; add sympy patch to remove UndefFunction._sage_
|
114733d | Merge branch 'u/rws/upgrade_to_sympy_1_0' of git://trac.sagemath.org/sage into t/20185/upgrade_to_sympy_1_0
|
76a47e8 | 20185: revert an unnecessary change
|
comment:22 follow-up: ↓ 26 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
- 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:
42dbc89 | Changing version to p0 and fixing doctest in unicode_art.py.
|
comment:25 Changed 5 years ago by
- 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 5 years ago by
...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.
comment:28 Changed 5 years ago by
Thanks for the review.
comment:29 Changed 5 years ago by
- Status changed from positive_review to needs_work
PDF docs fail
! Emergency stop. <inserted text> $ l.22231 \PYG{g+go}{ ⎮ √x}
comment:30 Changed 5 years ago by
- 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 5 years ago by
- 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 5 years ago by
- Status changed from positive_review to needs_work
$ rpm -q texlive texlive-2014-19.20140525_r34255.fc23.x86_64
comment:33 Changed 5 years ago by
- Commit changed from 42dbc8986991f8be078e0e4c61a2c55eb17febda to 3872f81c60ab0ae37d2ec64e240cce80a053edc8
Branch pushed to git repo; I updated commit sha1. New commits:
3872f81 | Fix U+221A in pdf docs
|
comment:34 Changed 5 years ago by
- 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 5 years ago by
- 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 5 years ago by
- Branch changed from public/packages/upgrade_sympy-20185 to 3872f81c60ab0ae37d2ec64e240cce80a053edc8
- Resolution set to fixed
- Status changed from positive_review to closed
Any hints?
New commits:
pkg version/chksum/patch removed