EmuKit / emukit

A Python-based toolbox of various methods in decision making, uncertainty quantification and statistical emulation: multi-fidelity, experimental design, Bayesian optimisation, Bayesian quadrature, etc.

https://emukit.github.io/emukit/

Apache License 2.0

605 stars 128 forks source link

Bump scipy version #315

Closed marpulli closed 4 years ago

marpulli commented 4 years ago

Just use latest scipy - it's causing conflicts with other projects to pin to such a specific version

ekalosak commented 4 years ago

Wow that introduced some weird bugs. I'm curious what changed in scipy that causes these tests to fail :\ Any ideas?

mmahsereci commented 4 years ago

I am not sure if this PR introduced the failure. The previous PR by Henry got the same error when calling check_grad (see approx_derivative in scipy.optimize._numdiff.py)

ekalosak commented 4 years ago

Gotcha. Thank you for clarifying @mmahsereci.

apaleyes commented 4 years ago

By looking at a few last builds, it looks as though at some point builds started pulling scipy 1.5 instead of 1.4, and started failing. Examples:

This PR, failed: Downloading scipy-1.5.2-cp37-cp37m-manylinux1_x86_64.whl (25.9 MB)
Build 3 months ago, succeeded: Downloading scipy-1.4.1-cp36-cp36m-macosx_10_6_intel.whl (28.5 MB)
PR submitted by @henrymoss, failed: Downloading scipy-1.5.1-cp36-cp36m-macosx_10_9_x86_64.whl (28.8 MB)

apaleyes commented 4 years ago

I have experimented locally and confirm that the error is caused by the scipy version. In the same virtual env, tests pass with scipy==1.4.1 and fail with scipy==1.5.1.

@marpulli sorry for hijacking the thread. Those interested in digging in, welcome to #316

apaleyes commented 4 years ago

@marpulli perhaps you were modifying a different version of the code... right now what I see in changes is going from scipy>=1.1.0 to scipy=>1. Which to me does not make much difference, and isn't very specific.

apaleyes commented 4 years ago

Ping @marpulli. Are you happy with how reqs file looks right now, or shall we continue with this PR?

codecov-io commented 4 years ago

Codecov Report

Merging #315 into master will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #315   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files         123      123           
  Lines        4019     4019           
  Branches      459      459           
=======================================
  Hits         3596     3596           
  Misses        328      328           
  Partials       95       95

Impacted Files	Coverage Δ
emukit/model_wrappers/gpy_model_wrappers.py	`83.33% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3c7c114...451dc6f. Read the comment docs.

marpulli commented 4 years ago

Hey, @apaleyes I was relaxing the constraint so that we would bump up to new minor versions automatically, but happy to set it to >=1.5 instead.

apaleyes commented 4 years ago

Sorry Mark, I really don't follow. What's wrong with just >=1.1.0 as it is now? Esp since we seem to have addressed the issue that was causing the tests to fail, so >=1.5 is not really necessary. Sorry for being slow on this one.

marpulli commented 4 years ago

It's me being slow 😆 . I was thinking that >=1.1.0 would allow any version 1.1.x but not 1.2.x but that doesn't seem to be true. Closing!

apaleyes commented 4 years ago

Phew! Thanks Mark! I actually had to do some research to convince myself this is the case. But seems like it is!