Upgrade to SymPy-1.0 - Githubissues

rwst commented 8 years ago

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.

Component: packages: standard

Author: Ralf Stephan

Branch/Commit: 3872f81

Reviewer: Travis Scrimshaw, Ralf Stephan, Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/20185

rwst commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 https://github.com/sympy/sympy/wiki/Release-Notes-for-1.0

-It could be installed as usually or via `pip install -U sympy`.
+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`.

rwst commented 8 years ago

Branch: u/rws/upgrade_to_sympy_1_0

rwst commented 8 years ago

Commit: 6971607

rwst commented 8 years ago

comment:3

$ ./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:

6971607 pkg version/chksum/patch removed

kiwifb commented 8 years ago

comment:4

Replying to @rwst:

$ ./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:

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

`9cdde13`	`20185: remove python -S stuff`
`907bedc`	`Merge branch 'develop' into t/20185/upgrade_to_sympy_1_0`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 6971607 to 907bedc

rwst commented 8 years ago

comment:6

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.

rwst commented 8 years ago

comment:7

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

f92f927 fix doctests

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 907bedc to f92f927

tscrim commented 8 years ago

comment:9

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from f92f927 to 0391252

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

0391252 fix doctest

rwst commented 8 years ago

comment:11

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

rwst commented 8 years ago

Author: Ralf Stephan

tscrim commented 8 years ago

comment:13

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

tscrim commented 8 years ago

comment:14

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

tscrim commented 8 years ago

comment:15

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.

tscrim commented 8 years ago

comment:16

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'>

tscrim commented 8 years ago

comment:17

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

rwst commented 8 years ago

comment:18

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

tscrim commented 8 years ago

comment:19

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.

rwst commented 8 years ago

comment:20

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 0391252 to 76a47e8

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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`

rwst commented 8 years ago

comment:22

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.

tscrim commented 8 years ago

comment:23

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.

tscrim commented 8 years ago

Reviewer: Travis Scrimshaw

tscrim commented 8 years ago

Changed commit from 76a47e8 to 42dbc89

tscrim commented 8 years ago

comment:24

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.

tscrim commented 8 years ago

Changed branch from u/rws/upgrade_to_sympy_1_0 to public/packages/upgrade_sympy-20185

rwst commented 8 years ago

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Ralf Stephan

rwst commented 8 years ago

comment:25

Your changes are fine. Consider them as reviewed.

rwst commented 8 years ago

comment:26

...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.

tscrim commented 8 years ago

comment:27

Thank you.

rwst commented 8 years ago

comment:28

Thanks for the review.

vbraun commented 8 years ago

comment:29

PDF docs fail

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

rwst commented 8 years ago

comment:30

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

tscrim commented 8 years ago

comment:31

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...

vbraun commented 8 years ago

comment:32

$ rpm -q texlive
texlive-2014-19.20140525_r34255.fc23.x86_64

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

3872f81 Fix U+221A in pdf docs

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 42dbc89 to 3872f81

vbraun commented 8 years ago

comment:34

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

tscrim commented 8 years ago

Changed reviewer from Travis Scrimshaw, Ralf Stephan to Travis Scrimshaw, Ralf Stephan, Volker Braun

tscrim commented 8 years ago

comment:35

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

vbraun commented 8 years ago

Changed branch from public/packages/upgrade_sympy-20185 to 3872f81

sagemath / sage

Upgrade to SymPy-1.0 #20185