Fix numpy version checks #3170

Merged
merged 6 commits into from Dec 26, 2013

Projects

None yet

3 participants

Owner

Closes gh-2998.

Note that no existing version string parsers in distutils/setuptools/distutils2 work well, so this is the simplest solution I think.

Coverage Status

Coverage remained the same when pulling 7bf2548 on rgommers:version-compare into 34ae412 on scipy:master.

Contributor

looks like __cmp__ is not the best way to do this with python3 http://docs.python.org/3.0/whatsnew/3.0.html#ordering-comparisons

Owner

Hmm, so have to implement six methods now instead of one. I'm sure there was a good reason, but it's not a very helpful decision here.

Coverage Status

Coverage remained the same when pulling 993aeb8 on rgommers:version-compare into 34ae412 on scipy:master.

Coverage Status

Coverage remained the same when pulling 63c7404 on rgommers:version-compare into 3982a90 on scipy:master.

Owner

OK fixed

@argriffing argriffing merged commit fde1826 into scipy:master Dec 26, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

Great! This addition looks like it could also be useful for other packages like sklearn

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/fixes.py#L82
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/preprocessing/label.py#L37
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/extmath.py#L155

or even upstream into numpy, or as a use case for distutils. By the way, I just noticed that NormalizedVersion seems to implement similar logic http://www.python.org/dev/peps/pep-0386/ maybe it could be used even though LooseVersion fails? It seems to know about alpha, beta, release candidate, development versions, etc.

[Edit: I just re-read the comments in the associated scipy issue, and I see that NormalizedVersion does not work for numpy versions]

Owner

Good idea to add this to numpy, we can stick it in numpy.version. Still needed in scipy to support older numpy versions then.

distutils is hibernating. Proposing an addition to NormalizedVersion would make sense I think but I don't have the bandwidth to argue about it, nor the patience to see my patch languish for a year or more.

Contributor

Still needed in scipy to support older numpy versions then.

Indeed...

I noticed https://github.com/scipy/scipy/blob/master/scipy/__init__.py#L83 should this be updated too?

@rgommers rgommers deleted the rgommers:version-compare branch Dec 26, 2013
Owner

Good catch, missed that one. Will also start printing an incorrect warning for numpy 1.10. Will send a new PR.

Contributor

Should this also be used to check scipy versions by packages that import scipy?

Owner

Yes, that would make sense. The next time that scipy version strings will go wrong is 0.20 (which I hope we won't reach). Would be best if NumpyVersion became a public class in numpy, then scipy users can re-use that.

Owner

Updated version check in gh-3177.

Owner

For future reference, here is the PEP that's about changing distutils.version: https://www.python.org/dev/peps/pep-0386/. It explains why StrictVersion / LooseVersion aren't ideal (to put it mildly).

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