Repository navigation
BUG: Added exception for casting numpy.ma.masked to long - #9817
Conversation
|
Needs a test, and the commit messages should start with |
kenogo
changed the title
|
@eric-wieser Alright. Since the |
|
Okay this should be it |
|
Oh damn it, I'm tired. First accidentally named the test function |
|
Ok last commit message is now edited accordingly. |
There was a problem hiding this comment.
This test is kinda unnecessary, as it actually tests exactly the same thing as above. long(np.ma.masked) would be a good test to have somewhere though.
Also, you want skipif, not knownfailureif here - the latter is reserved for things that make sense to fix in future.
There was a problem hiding this comment.
So if I test with an int in nose, it automatically tests int for Python 3 and int and long for Python 2? This means I should just get rid of my new testing function and I'm done? I was already wondering why you added a knownfailureif decorator for test_coercion_int, this explains it.
There was a problem hiding this comment.
nose isn't doing anything special here.
The old bug was not actually caused by long there, but by any integer type, even np.int8 - because internally, numpy was calling long.
It would still be good to have a long(np.ma.masked) test somewhere. I'd hope we already have one for int which it could go alongside, but I might be overly optimistic there.
There was a problem hiding this comment.
The fix itself looks fine, by the way :)
|
So is there anything left to do for me or can this be merged? |
|
See the hidden comment above |
|
@eric-wieser Oh I totally overread that. I hope I got what you wanted correctly... Please elaborate if I misunderstood you. |
There was a problem hiding this comment.
Better as assert_raises(MaskError, int, np.ma.masked). Here you do need a test for long as well.
I think there might already be a test for this kind of thing elsewhere. Grepping for assert_raises(MaskError ought to find it
There was a problem hiding this comment.
@eric-wieser Nope a test for this doesn't exist yet.
|
That's it I think |
|
Looks good to me. Those 6 commits need squashing into one or two (there aren't 6 different changes being made here), but I can do that if you don't want to. |
|
@eric-wieser Yeah I'm not sure how to do that. Go ahead. |
TST: Added test_coercion_long and removed knownfailure from test_coercion_int
|
@eric-wieser Nevermind, I figured it out :) |
|
Thanks for taking the time to learn how to do that, and thanks for the contribution :) |
This fixes #9751.