Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to SymPy-1.0 #20185

Closed
rwst opened this issue Mar 10, 2016 · 47 comments
Closed

Upgrade to SymPy-1.0 #20185

rwst opened this issue Mar 10, 2016 · 47 comments

Comments

@rwst
Copy link

rwst commented Mar 10, 2016

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 rwst added this to the sage-7.1 milestone Mar 10, 2016
@rwst

This comment has been minimized.

@rwst
Copy link
Author

rwst commented Mar 10, 2016

Branch: u/rws/upgrade_to_sympy_1_0

@rwst
Copy link
Author

rwst commented Mar 10, 2016

Commit: 6971607

@rwst
Copy link
Author

rwst commented Mar 10, 2016

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:

6971607pkg version/chksum/patch removed

@kiwifb
Copy link
Member

kiwifb commented Mar 11, 2016

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:

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 11, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 11, 2016

Changed commit from 6971607 to 907bedc

@rwst
Copy link
Author

rwst commented Mar 11, 2016

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
Copy link
Author

rwst commented Mar 11, 2016

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 11, 2016

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

f92f927fix doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 11, 2016

Changed commit from 907bedc to f92f927

@tscrim
Copy link
Collaborator

tscrim commented Mar 11, 2016

comment:9

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 11, 2016

Changed commit from f92f927 to 0391252

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 11, 2016

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

0391252fix doctest

@rwst
Copy link
Author

rwst commented Mar 11, 2016

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 sympy/sympy#10795

@rwst
Copy link
Author

rwst commented Mar 11, 2016

Author: Ralf Stephan

@tscrim
Copy link
Collaborator

tscrim commented Mar 11, 2016

comment:13

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 11, 2016

comment:14

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 12, 2016

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
Copy link
Collaborator

tscrim commented Mar 12, 2016

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
Copy link
Collaborator

tscrim commented Mar 12, 2016

comment:17

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

@rwst
Copy link
Author

rwst commented Mar 13, 2016

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
Copy link
Collaborator

tscrim commented Mar 13, 2016

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
Copy link
Author

rwst commented Mar 13, 2016

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 13, 2016

Changed commit from 0391252 to 76a47e8

@rwst
Copy link
Author

rwst commented Mar 21, 2016

comment:30

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2016

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
Copy link
Member

vbraun commented Mar 22, 2016

comment:32
$ rpm -q texlive
texlive-2014-19.20140525_r34255.fc23.x86_64

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2016

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

3872f81Fix U+221A in pdf docs

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2016

Changed commit from 42dbc89 to 3872f81

@vbraun
Copy link
Member

vbraun commented Mar 22, 2016

comment:34

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2016

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 22, 2016

comment:35

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

@vbraun
Copy link
Member

vbraun commented Mar 23, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants