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

BUG: Fix failing stats tests #9681

Merged
merged 1 commit into from Jan 14, 2019
Merged

BUG: Fix failing stats tests #9681

merged 1 commit into from Jan 14, 2019

Conversation

@larsoner
Copy link
Member

larsoner commented Jan 14, 2019

Pending the result of the related NumPy issue

numpy/numpy#12737

This might not be the right fix. But in the meantime this should get Travis happy again, so probably worth merging and following up on later if necessary.

The list({...}) change fixes a bug that I see locally using latest numpy where np.vstack gives:

E FutureWarning: arrays to stack must be passed as a "sequence" type such as list or tuple. Support for non-sequence iterables such as generators is deprecated as of NumPy 1.16 and will raise an error in the future.

scipy/stats/stats.py Outdated Show resolved Hide resolved
@larsoner

This comment has been minimized.

Copy link
Member Author

larsoner commented Jan 14, 2019

So the change was on purpose. I guess the question is do we want to still emit a warning here? My guess is "yes", so I'll modify the code to emit our own warning.

@ev-br

This comment has been minimized.

Copy link
Member

ev-br commented Jan 14, 2019

I'd suggest following whatever numpy does, and just stop checking the number of warnings. It's not "our" warning, so the assertion seems extraneous to me.

@larsoner larsoner force-pushed the larsoner:fix-stats branch 2 times, most recently from 0c4b48f to 5b8e618 Jan 14, 2019
@larsoner larsoner force-pushed the larsoner:fix-stats branch from 5b8e618 to 56c1ee0 Jan 14, 2019
@larsoner

This comment has been minimized.

Copy link
Member Author

larsoner commented Jan 14, 2019

Okay, got rid of the warning count check. Okay to merge once CIs come back happy (other than Pypy3, which has also been failing for a week or so unrelatedly)?

@ev-br

This comment has been minimized.

Copy link
Member

ev-br commented Jan 14, 2019

Yeah, pypy failure seems inrelated, +1 from me for merging as is.

@ilayn

This comment has been minimized.

Copy link
Member

ilayn commented Jan 14, 2019

I agree with @ev-br

@larsoner

This comment has been minimized.

Copy link
Member Author

larsoner commented Jan 14, 2019

Thanks for the quick reviews, in it goes

@ev-br ev-br merged commit 22a1a8a into scipy:master Jan 14, 2019
4 of 5 checks passed
4 of 5 checks passed
ci/circleci: pypy3 Your tests failed on CircleCI
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scipy.scipy Build #20190114.17 succeeded
Details
@larsoner

This comment has been minimized.

Copy link
Member Author

larsoner commented Jan 14, 2019

Beat me to it :)

@ev-br

This comment has been minimized.

Copy link
Member

ev-br commented Jan 14, 2019

Did not mean to race for it :-).
Thanks for a quick fix.

@mdhaber

This comment has been minimized.

Copy link
Contributor

mdhaber commented Jan 14, 2019

Thanks!

@@ -1983,7 +1983,9 @@ def test_scale(self):
np.array([2, np.nan, 2]) / 1.3489795)
assert_equal(stats.iqr(x, axis=1, scale=2.0, nan_policy='propagate'),
[1, np.nan, 1])
_check_warnings(w, RuntimeWarning, 6)
# Since NumPy 1.17.0.dev, warnings are no longer emitted by

This comment has been minimized.

Copy link
@madphysicist

madphysicist Jan 14, 2019

Contributor

I realize that this has already been merged, but I would prefer to keep the check, just wrapped in if numpy_version < '1.16.0':

This comment has been minimized.

Copy link
@madphysicist

madphysicist Jan 14, 2019

Contributor

My reason is that IQR is extremely sensitive to the numpy version number, so I would like to retain all the version-specific checking in the tests. I think it might be even better to do something more explicit like _check_warnings(w, RuntimeWarning, 0 if numpy_version >= '1.16.0a' else 6).

This comment has been minimized.

Copy link
@larsoner

larsoner Jan 14, 2019

Author Member

That's how I originally had the check, at least as:

n_want = 0 if numpy_version >= '1.17.0.dev' else 6

assuming they are not backporting the nan changes to the 1.16 branch

I don't have a preference either way

This comment has been minimized.

Copy link
@madphysicist

madphysicist Jan 14, 2019

Contributor

If it's not too much to reinstate that, I think it will be preferable in the long run. I think versions will give this function a lot more grief in the future, and it's important to retain traceability. The development process still gives me shivers.

tylerjereddy added a commit to tylerjereddy/scipy that referenced this pull request Jan 15, 2019
@tylerjereddy

This comment has been minimized.

Copy link
Contributor

tylerjereddy commented Jan 15, 2019

I've tried to partially cherry-pick this PR for a backport in #9683. The reason for partial backport is that only the vstack-related change seems to have been causing a test failure. If I need the rest I can include it, but should be good reason first.

@larsoner

This comment has been minimized.

Copy link
Member Author

larsoner commented Jan 15, 2019

Whatever gets everything green! I'd expect you to need both fixes from this PR at some point, though, since they both fix changes in either behavior or emitted warnings on latest NumPy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.