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

NumpyVersion does not handle the post suffix correctly. #6431

Closed
charris opened this issue Oct 9, 2015 · 14 comments
Closed

NumpyVersion does not handle the post suffix correctly. #6431

charris opened this issue Oct 9, 2015 · 14 comments

Comments

@charris
Copy link
Member

charris commented Oct 9, 2015

In [1]: from numpy.lib import NumpyVersion

In [2]: NumpyVersion('1.10.0.post2') >= '1.10.0'
Out[2]: False

In [3]: NumpyVersion('1.10.0.post2') >= '1.10.0.post1'
Out[3]: True

In [4]: NumpyVersion('1.10.0.post2') > '1.10.0.post1'
Out[4]: False

In [5]: NumpyVersion('1.10.0.post1') > '1.10.0'
Out[5]: False

In [6]: NumpyVersion('1.10.0.post1') == '1.10.0'
Out[6]: False

In [7]: NumpyVersion('1.10.0.post1') < '1.10.0'
Out[7]: True
@sumitbinnani
Copy link

@charris
Is anyone already looking into it? If not, I would like to submit the patch :-)

@njsmith
Copy link
Member

njsmith commented Oct 9, 2015

You can, but I'm not sure what the right solution is :-)

Option 1 would be to just never use the .post suffix again, so the problem
goes away :-)

Option 2 would be to find and steal an existing PEP 440 implementation from
somewhere (distlib?)

Option 3 would be to hack up NumpyVersion to handle what we need directly.

On Thu, Oct 8, 2015 at 11:18 PM, Sumit Binnani notifications@github.com
wrote:

Can I work on it?


Reply to this email directly or view it on GitHub
#6431 (comment).

Nathaniel J. Smith -- http://vorpus.org

@rgommers
Copy link
Member

rgommers commented Oct 9, 2015

+1 for option 1.

option 2: I looked quite hard for an implementation that did the right thing before writing NumpyVersion. I never even considered checking for .postN, and I have never seen it used in any project.

(and given that NumpyVersion is for comparing numpy versions and not a complete PEP440 implementation, it wasn't a bug until a few days ago)

@charris
Copy link
Member Author

charris commented Oct 9, 2015

After rereading PEP440 a couple of times, I'm inclined towards 1. also. PEP440 was too flexible, making it difficult to understand, let alone to parse, the allowable versions. It should have come with a reference implementation as a demonstration of concept ;)

@njsmith
Copy link
Member

njsmith commented Oct 9, 2015

Supposedly there is a reference implementation somewhere -- it was in the
pypa repo but now I can't find it. Maybe they moved it into distlib or pip
or something. Anyway, doesn't much matter.
On Oct 9, 2015 8:14 AM, "Charles Harris" notifications@github.com wrote:

After rereading PEP440 a couple of times, I'm inclined towards 1. also.
PEP440 was too flexible, making it difficult to understand, let alone
to parse, the allowable versions. It should have come with a reference
implementation as a demonstration of concept ;)


Reply to this email directly or view it on GitHub
#6431 (comment).

@charris
Copy link
Member Author

charris commented Oct 9, 2015

@rgommers I have seen .postN a couple of times, don't recall where, but it seems to be a new option and hence not represented in the geological strata.

@rgommers
Copy link
Member

So can we close this as wontfix / not a bug?

@charris
Copy link
Member Author

charris commented Oct 10, 2015

Might a well.

@charris charris closed this as completed Oct 10, 2015
@njsmith
Copy link
Member

njsmith commented Oct 10, 2015

Should we add a line or two of code to make import bomb out when the
version number isn't recognized, both to enforce the decision that we won't
use those numbers and remind anyone who tries why not?
On Oct 10, 2015 5:16 AM, "Charles Harris" notifications@github.com wrote:

Closed #6431 #6431.


Reply to this email directly or view it on GitHub
#6431 (comment).

@charris
Copy link
Member Author

charris commented Oct 10, 2015

@njsmith That would be appropriate.

@rgommers
Copy link
Member

That sounds good. Anything other than #.##.### should start with .dev0+ and then a git hash or Unknown.

@njsmith
Copy link
Member

njsmith commented Oct 10, 2015

So as a regex, does this look right:

^[0-9]+.[0-9]+.[0-9]+(.dev0+([0-9a-f]+|Unknown))?$

?
On Oct 10, 2015 11:12 AM, "Ralf Gommers" notifications@github.com wrote:

That sounds good. Anything other than #.##.### should start with .dev0+
and then a git hash or Unknown.


Reply to this email directly or view it on GitHub
#6431 (comment).

@rgommers
Copy link
Member

Actually no, because my description above is wrong. It doesn't allow for alpha/beta/rc version numbers. Should add |a[0-9]|b[0-9]|rc[0-9]

@jensenbox
Copy link

FYI: This is breaking the statsmodels package - could be others affected: statsmodels/statsmodels#2645

rgommers added a commit to rgommers/numpy that referenced this issue Jan 16, 2016
jaimefrio pushed a commit to jaimefrio/numpy that referenced this issue Mar 22, 2016
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

5 participants