[MRG] test against numpy dev #5609
Conversation
|
Those tests error because they give warnings that should be filtered out, and numpy master converts I don't see these warnings locally, but it's easy to filter with |
|
|
e974097 to
df87dd4
Compare
|
I can reproduce the runtime warning with numpy master. Will have to investigate. |
|
It's not at all visible now that the test actually failed. I suspect that no one will now look at the test result against numpy master, and hence it doesn't help much. So I guess just failing is better - if numpy master has a regression that takes a while to fix, we can temporarily disable the test run. Does that make sense? |
df87dd4 to
577ecb9
Compare
|
@rgommers ok I removed the |
577ecb9 to
8678b4b
Compare
|
I have amended my commit to use the shorter travis-dev-wheels.scipy.org URL instead of the long rackcdn.com URL. If the tests pass, I think this is ready for merge. The real upload mechanism can only be tested once we merge this PR to master though. |
|
While it's a minor quibble, I do think it's better to filter the warnings in the tests rather than the library code (#5621 (comment) lists the exact warnings). |
|
@ev-br you mean catch the warnings using e.g. |
|
IIUC the specific issue here is that scipy is using (This is of course separate from the more general issue of how we want to handle breakage caused by numpy skew, allow_failure, etc.) |
|
@ev-br IMHO, With numpy 1.10.2 the following does not raise a warning: >>> from scipy import stats
>>> stats.spearmanr([1.0, 2.0, 3.0, 4.0], [1.0, 2.0, 2.0, 3.0])
SpearmanrResult(correlation=0.94868329805051377, pvalue=0.051316701949486225)while it does with numpy master. The problem with the fact that |
|
For reference here is the behavior observed in a shell (outside of the testing environment) with numpy master: >>> import numpy as np
>>> from scipy import stats
>>> stats.spearmanr([1.0, 2.0, 3.0, 4.0], [1.0, 2.0, 2.0, 3.0])
/Users/ogrisel/code/scipy/scipy/stats/stats.py:3314: RuntimeWarning: invalid value encountered in sqrt
t = rs * np.sqrt((n-2) / ((rs+1.0)*(1.0-rs)))
/Users/ogrisel/code/scipy/scipy/stats/_distn_infrastructure.py:1748: RuntimeWarning: invalid value encountered in greater
cond1 = (scale > 0) & (x > self.a) & (x < self.b)
/Users/ogrisel/code/scipy/scipy/stats/_distn_infrastructure.py:1748: RuntimeWarning: invalid value encountered in less
cond1 = (scale > 0) & (x > self.a) & (x < self.b)
8000
/Users/ogrisel/code/scipy/scipy/stats/_distn_infrastructure.py:1749: RuntimeWarning: invalid value encountered in less_equal
cond2 = cond0 & (x <= self.a)
SpearmanrResult(correlation=0.94868329805051388, pvalue=0.051316701949486114)I find this really confusing for the user. The input is valid. The fact that the internals of Here are the details of an ipdb session on numpy master: The close to zero negative value on the diagonal looks like a usual rounding issue to me. |
|
Here is an alternative fix that does not change the warning handling: diff --git a/scipy/stats/stats.py b/scipy/stats/stats.py
index 505fe92..36b44ca 100644
--- a/scipy/stats/stats.py
+++ b/scipy/stats/stats.py
@@ -3311,7 +3311,8 @@ def spearmanr(a, b=None, axis=0, nan_policy='propagate'):
olderr = np.seterr(divide='ignore') # rs can have elements equal to 1
try:
- t = rs * np.sqrt((n-2) / ((rs+1.0)*(1.0-rs)))
+ tmp = (n - 2) / ((rs + 1.0) * (1.0 - rs))
+ t = rs * np.sqrt(tmp.clip(0))
finally:
np.seterr(**olderr)I can update my PR if you think this is the right fix. |
|
Yeah, the ideal would actually be that |
|
@njsmith I agree. The behavior of the numpy warnings could be configured in the runtests.py utility, maybe with additional commandline flags. |
8678b4b to
5c18776
Compare
|
I went on and pushed the clip-based fix. Let me know what you think about it. |
|
OK, spearmanr glitch is somewhat parallel to uploading wheels. I'll merge this to |
|
Thanks! |
|
See also numpy/numpy#6901, which changes numpy's test-runner to always default to |
Our test-runner's raise_warning mode traditionally has varied depending on whether we have a development or release version of numpy: for development versions we raise on warnings, and for release versions we don't. This is all very sensible... *if* you're running numpy's test suite. But our test-runner is also used by other packages like scipy, and it doesn't make sense for scipy's raise_warning mode to vary depending on whether *numpy* is a development or release version. (It should vary depending on whether the scipy-under-test is a development or release version.) So this commit moves the numpy-version-dependent raise_warning logic out of the generic NoseTester class and into numpy-specific code. (See scipy/scipy#5609 for more discussion.)
We want CI tests to fail when they use deprecated functionality or otherwise raise spurious warnings, so enable raise_warnings="develop" for development builds. See scipygh-5609 for related discussion, and numpy/numpy#6901 for a related change in numpy.
|
Great to have this merged. I see that Python 3.5 wheels are appearing at http://travis-dev-wheels.scipy.org/. Something is not quite right there though - only the very latest wheel should be kept. Now pip will have no way of knowing what the most recent one is right? |
|
IIUC it keeps the most recent 5, and inserts a timestamp into the version
|
|
Does pip sort the file names? Or is the part following |
|
IIRC If there are only 5 files, they'll all be recent. But it's still possible that you would fetch a numpy and a scipy wheel from the wheelhouse that are incompatible - there's a pretty decent chance of that happening actually at the moment that numpy adds something to its API. |
|
Interesting question! I can't find anything definitive in a quick search, but https://bitbucket.org/pypa/pypi-metadata-formats/issues/37/ordering-of-local-segments suggests that at some point the relevant people at least decided that pip should sort the local version specifiers in the way that we want. |
|
Based on my tests pip picks up the latest version including some deterministic ordering on the local version segment which is why I made By default it keeps the last 5 most recent dev builds (per-project / python version / platform tags). So this should all be fine. |
|
okay, thanks for confirming @ogrisel |
Our test-runner's raise_warning mode traditionally has varied depending on whether we have a development or release version of numpy: for development versions we raise on warnings, and for release versions we don't. This is all very sensible... *if* you're running numpy's test suite. But our test-runner is also used by other packages like scipy, and it doesn't make sense for scipy's raise_warning mode to vary depending on whether *numpy* is a development or release version. (It should vary depending on whether the scipy-under-test is a development or release version.) So this commit moves the numpy-version-dependent raise_warning logic out of the generic NoseTester class and into numpy-specific code. (See scipy/scipy#5609 for more discussion.)
This is work in progress for a fix for #5379 building upon the work done for numpy/numpy#6493.
/The current scipy test fails with:It might be revealing a regression in numpy master or it might be caused by something that I don't understand.I am wondering if we should put this numpy dev entry of the scipy build matrix in allow_failure.I have not had the opportunity to test the upload section of this PR yet as the tests fail.