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 NumPy 1.15.2 #25260

Closed
slel opened this issue Apr 29, 2018 · 101 comments
Closed

Upgrade to NumPy 1.15.2 #25260

slel opened this issue Apr 29, 2018 · 101 comments

Comments

@slel
Copy link
Member

slel commented Apr 29, 2018

NumPy 1.15.2 was released.

Tarball: https://files.pythonhosted.org/packages/45/ba/2a781ebbb0cd7962cc1d12a6b65bd4eff57ffda449fdbbae4726dc05fbc3/numpy-1.15.2.zip

Our last upgrade was to NumPy 1.13.3 in #24063.

A bug introduced in NumPy 1.15 prevents building NumPy on machines with many CPUs.

The issue is reported and solved upstream:

Upstream: Fixed upstream, but not in a stable release.

CC: @kiwifb @infinity0 @jdemeyer @slel @sagetrac-epalezzato

Component: packages: standard

Keywords: upgrade, numpy

Author: Timo Kaufmann, Julian Rüth, Bryan Gin-ge Chen

Branch: b151634

Reviewer: Travis Scrimshaw

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

@slel slel added this to the sage-8.3 milestone Apr 29, 2018
@slel
Copy link
Member Author

slel commented Apr 29, 2018

comment:1

For NumPy 1.13.3 we have the tarball in .zip, should we also use the .zip this time or the .tar.gz?

@slel

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Apr 29, 2018

comment:2

We now support zip files I believe, so this is fine.

@jdemeyer
Copy link

comment:3

If we support both kinds of archive, we should probably use the smallest one.

@timokau
Copy link
Contributor

timokau commented May 7, 2018

comment:4

I don't know how or if I can add changes here, but I have fixed the doctests for the numpy+scipy upgrade here.

Its mostly formatting. Interesting bits:

  • here I avoid a numpy deprecation warning (empty array should not be used as false) by always making sure points is a regular array and then comparing its len.
  • the iprint kwarg in fmin_cobyla is deprecated and repaced by disp

Edit: I was a bit too fast and didn't test it properly. I'll post an updated version once thats tested.

@timokau
Copy link
Contributor

timokau commented May 8, 2018

comment:5

I have updated the patch.
The doctests should pass now.

@slel
Copy link
Member Author

slel commented Jun 10, 2018

comment:6

Changing target from Numpy 1.14.3 to 1.14.4.

@slel

This comment has been minimized.

@timokau
Copy link
Contributor

timokau commented Jul 8, 2018

Commit: 3322ff1

@timokau
Copy link
Contributor

timokau commented Jul 8, 2018

Branch: public/25260

@timokau
Copy link
Contributor

timokau commented Jul 8, 2018

comment:7

I updated the package and applied my doctest fixes. make ptestlong passes.


New commits:

3322ff1Upgrade numpy to 1.14.4

@slel

This comment has been minimized.

@slel
Copy link
Member Author

slel commented Jul 25, 2018

comment:8

Numpy 1.15.0 just came out.

@slel slel modified the milestones: sage-8.3, sage-8.4 Jul 25, 2018
@slel slel changed the title Upgrade to NumPy 1.14.3 Upgrade to NumPy 1.15.0 Jul 25, 2018
@jdemeyer
Copy link

jdemeyer commented Aug 4, 2018

Dependencies: #25755

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2018

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

ecf870bMerge remote-tracking branch 'origin/develop' into u/gh-timokau/numpy-1.14.4
aede54dUpgrade numpy to 1.15.0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2018

Changed commit from 3322ff1 to aede54d

@timokau
Copy link
Contributor

timokau commented Aug 5, 2018

comment:12

I've done most of the upgrade, but two failures in src/sage/plot/histogram.py remain. I think both are rooted in the following issue:

> import numpy
> numpy.histogram([], range=(1.0,1.0))

TypeError                                 Traceback (most recent call last)
<ipython-input-2-05b803a2f80b> in <module>()
----> 1 numpy.histogram([], range=(RealNumber('1.0'),RealNumber('1.0')))

/home/timo/repos/sage/local/lib/python2.7/site-packages/numpy/lib/histograms.pyc in histogram(a, bins, range, normed, weights, density)
    674     a, weights = _ravel_and_check_weights(a, weights)
    675 
--> 676     bin_edges, uniform_bins = _get_bin_edges(a, bins, range, weights)
    677 
    678     # Histogram is an integer or a float array depending on the weights.

/home/timo/repos/sage/local/lib/python2.7/site-packages/numpy/lib/histograms.pyc in _get_bin_edges(a, bins, range, weights)
    342         # shapes. To avoid this causing problems, we pick a type now and stick
    343         # with it throughout.
--> 344         bin_type = np.result_type(first_edge, last_edge, a)
    345         if np.issubdtype(bin_type, np.integer):
    346             bin_type = np.result_type(bin_type, float)

TypeError: data type not understood

This does not happen outside of sage. It probably has something to do with sages custom number types.

@jdemeyer
Copy link

jdemeyer commented Aug 6, 2018

Changed dependencies from #25755 to #25702

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2018

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

a5707a1Merge in latest develop branch
b584304Fix incorrect output in old docstring
6abfb96fix spacing
27a7f75A historgram's range must be a Python float

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2018

Changed commit from aede54d to 27a7f75

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2018

Changed commit from 78057db to 40f4318

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2018

Changed commit from 40f4318 to 3508a05

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3508a05Limit number of threads to prevent bombing the system

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2018

Changed commit from 3508a05 to b151634

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b151634Limit number of threads to prevent bombing the system

@timokau
Copy link
Contributor

timokau commented Oct 5, 2018

comment:69

Just for the record I would prefer to monkey-patch instead (or maybe additionally). It seems to be a clear bug in this case and downstream distributions should probably patch, but I still think sage should do what it can to prevent errors even on an unpatched system.

I know your views on this but still wanted to make that point.

@saraedum
Copy link
Member

saraedum commented Oct 6, 2018

comment:70

gh-timokau, I would usually agree that we should monkey-patch. However, in this case, there seems to be nothing Sage specific going on so I do not see a strong reason to monkey-patch. In particular, this does not completely break Sage but just breaks things in some rare cases. As this is a numpy bug, distributions should backport it in their numpy if they care.

Probably more importantly, this would be somewhat hard to monkey-patch. Numpy is not imported in a central spot in Sage as the import takes a while, so it is only loaded on demand. Therefore, there is not a single spot where the monkey-patching could happen.

@timokau
Copy link
Contributor

timokau commented Oct 6, 2018

comment:71

Yes, I agree that the argument is a bit weak in this case. If it was easy I would still prefer it, but since its not its not that bad.

@jdemeyer
Copy link

jdemeyer commented Oct 9, 2018

Upstream: Fixed upstream, but not in a stable release.

@jdemeyer
Copy link

jdemeyer commented Oct 9, 2018

comment:72

The patch was accepted upstream.

@slel

This comment has been minimized.

@slel
Copy link
Member Author

slel commented Oct 9, 2018

comment:73

Should we then upgrade to NumPy 1.14.6 now, or wait for NumPy 1.15.3 to be released?

@timokau
Copy link
Contributor

timokau commented Oct 9, 2018

comment:74

From a packaging perspective its preferable to upgrade to the latest version now. The latest version just requires a patch for some edge case. Without the upgrade sage is completely incompatible with newer numpy versions (which distros probably have by now).

Thank you for your efforts in fixing this upstream Jeroen!

@vbraun
Copy link
Member

vbraun commented Oct 20, 2018

Changed branch from public/25260 to b151634

@embray
Copy link
Contributor

embray commented Oct 28, 2018

comment:76

This should be re-targeted for 8.5.

@embray embray modified the milestones: sage-8.4, sage-8.5 Oct 28, 2018
@slel
Copy link
Member Author

slel commented Nov 5, 2018

Changed commit from b151634 to none

@slel
Copy link
Member Author

slel commented Nov 5, 2018

comment:77

Next NumPy upgrade at #26643.

@alexjbest
Copy link
Contributor

comment:78

I get two doctest failures on OSX with SageMath version 8.5.beta2, which I believe are related to this ticket, please let me know what other information is helpful (or if I should ignore this).

File "src/sage/schemes/elliptic_curves/height.py", line 1629, in sage.schemes.elliptic_curves.height.EllipticCurveCanonicalHeight.wp_on_grid
Failed example:
    H.wp_on_grid(v,4)
Expected:
    array([[25.43920182,  5.28760943,  5.28760943, 25.43920182],
           [ 6.05099485,  1.83757786,  1.83757786,  6.05099485],
           [ 6.05099485,  1.83757786,  1.83757786,  6.05099485],
           [25.43920182,  5.28760943,  5.28760943, 25.43920182]])
Got:
    array([[ 25.43920182,   5.28760943,   5.28760943,  25.43920182],
           [  6.05099485,   1.83757786,   1.83757786,   6.05099485],
           [  6.05099485,   1.83757786,   1.83757786,   6.05099485],
           [ 25.43920182,   5.28760943,   5.28760943,  25.43920182]])
**********************************************************************
File "src/sage/schemes/elliptic_curves/height.py", line 1637, in sage.schemes.elliptic_curves.height.EllipticCurveCanonicalHeight.wp_on_grid
Failed example:
    H.wp_on_grid(v,4,True)
Expected:
    array([[25.43920182,  5.28760943],
           [ 6.05099485,  1.83757786],
           [ 6.05099485,  1.83757786],
           [25.43920182,  5.28760943]])
Got:
    array([[ 25.43920182,   5.28760943],
           [  6.05099485,   1.83757786],
           [  6.05099485,   1.83757786],
           [ 25.43920182,   5.28760943]])
**********************************************************************

@kiwifb
Copy link
Member

kiwifb commented Nov 11, 2018

comment:79

I don't think it is the first time we have a formatting issue of that kind but I am not sure how to deal with it. The first thing that comes to mind is: are you using OS X' clang or sage's gcc?

@alexjbest
Copy link
Contributor

comment:80

I'm not 100% sure I'm looking in the right place but looks like homebrew gcc, version 8.2.0. As far as I know I haven't tried to change to clang for my sage install.

I have this line in config.log Configured with: ../configure --build=x86_64-apple-darwin17.7.0 --prefix=/usr/local/Cellar/gcc/8.2.0 --libdir=/usr/local/Cellar/gcc/8.2.0/lib/gcc/8 --enable-languages=c,c++,objc,obj-c++,fortran --program-suffix=-8 --with-gmp=/usr/local/opt/gmp --with-mpfr=/usr/local/opt/mpfr --with-mpc=/usr/local/opt/libmpc --with-isl=/usr/local/opt/isl --with-system-zlib --enable-checking=release --with-pkgversion='Homebrew GCC 8.2.0' --with-bugurl=https://github.com/Homebrew/homebrew-core/issues --disable-nls

@kiwifb
Copy link
Member

kiwifb commented Nov 11, 2018

comment:81

Hum... Not sure. You should open a new ticket in any case. While numpy could be your culprit if numpy arrays are used it could be something else entirely.

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

9 participants