-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
BUG: fix some errors raised when minlength is incorrect in np.bincount #4542
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
Conversation
else if (!(minlength = PyArray_PyIntAsIntp(mlength))) { | ||
if (mlength == Py_None) { | ||
minlength = 0; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}
else {
Looks OK, needs some tests. @jaimefrio has been working on this function also. |
@immerrr After the tests go in, squash Minor fixes into the first commit. |
@charris sure. I intended to check with "assert_raises_regexp" instead of "assert_raises", because some errors had incorrect messages. Is this OK to pass this function from |
I was thinking you could just check the error type, but checking the messages might be better. The potential problem with checking messages is that they are more likely to get edited, but that probably wouldn't be hard to fix when needed. I don't see a problem with exporting |
Stack exchange yields this little nugget,
That's neat. We might want to directly export |
Some experimentation
|
Agreed on that. My usual problem with
I usually don't like the extra noise added by assert_raises_regexp(Exception, "pattern", call_your_method, p1, p2)
# or
assert_raises_regexp(Exception, "pattern", lambda: call_your_method(p1, p2)) I'm going to add necessary tests tonight. |
Great. |
Tests are failing, looks like a failure to import nose correctly. I suspect it has to do with the |
However, for some reason it seems to work for python 2.7. |
Looks like nose 1.3.0 is using a deprecated function in assert_raises_regexp. Evidently python 2.7 isn't raising that warning. Grrr @NoSe. |
@@ -1546,6 +1546,23 @@ def test_empty_with_minlength(self): | |||
y = np.bincount(x, minlength=5) | |||
assert_array_equal(y, np.zeros(5, dtype=int)) | |||
|
|||
def test_with_incorrect_minlength(self): | |||
x = np.array([], dtype=int) | |||
assert_raises_regexp(TypeError, "an integer is required", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is raising a deprecation warning in nose 1.3.0 which numpy testing turns into an exception. Try self.assertRaisesRegexp
instead.
Nuts. The
Or maybe it is easier to implement our own |
Looks like we can copy the function from unittest. We need
And
In Python 3.1 and above this is called |
Ok, will rename to But this really looks like it can be fixed at nose level to avoid all these python-specific hacks in each project... |
The main problem is that nose uses unittest and the function isn't available in Python 2.6. Python 2.6 is in both RHEL and one of the Ubuntu long term support releases, so we will need to support it for maybe four more years. |
Supposedly the six compatibility package would take care of the python 3 problem, but I didn't find the function in its code. There is a unittest2 package on pypi that backports all the new functions introduced in Python 2.7 to earlier versions, but after chasing this around, it looked easier to just include the code for the one function rather than get another dependency. |
Well done! Thanks @immerrr. |
BUG: fix some errors raised when minlength is incorrect in np.bincount
BTW, there was also an |
It is funny that I did mention it in nose-devs/nose#789, but didn't think of bringing its goodness to numpy. |
I've encountered weird exceptions raised from
np.bincount
whenminlength
is incorrect. It turns out,PyArray_PyIntAsIntp
return values were handled incorrectly in that function, this PR is an attempt to fix this.I'd also argue that
minlength=0
is a valid input, but that would be an API change and hence worth a separate discussion.Before
After