DOC, MAINT: Enforce np.ndarray arg for np.put and np.place#7000
DOC, MAINT: Enforce np.ndarray arg for np.put and np.place#7000shoyer merged 1 commit intonumpy:masterfrom gfyoung:ndarray_arg_enforce
Conversation
|
Could someone explain to me what the purpose of the failing test ( |
numpy/core/fromnumeric.py
Outdated
There was a problem hiding this comment.
My preference for ducktyping would suggest using a try/except? I.e.,
try:
return a.put(ind, v, mode)
except AttributeError:
if not isinstance(a, np.ndarray):
raise TypeError(...)
raise
This should also be a little faster for the most common case, where a is in fact an ndarray.
There was a problem hiding this comment.
Hmmm...I'm a little on the fence on this one. While I do agree it should be faster in the most common case (though how much faster I'm not really sure), coupling the type checking with the function call makes us dependent on an AttributeError when calling the C API should a not be an ndarray. Should the API change, say to something like what is done with np.place, that try...except would become moot.
There was a problem hiding this comment.
I would suggest using the _wrapit functionality here, as used elsewhere in this file. _wrapit uses duck typing, similarly to @mhvk's suggestion.
There was a problem hiding this comment.
@shoyer : IMHO using _wrapit does not help here because I want to enforce that an ndarray must be passed in. Because np.put modifies the ndarray in-place, passing in a non-ndarray object will do nothing (the array_like object itself is not changed because it has to be cast to an ndarray object first) and will not raise any errors / warnings, which is what I am trying to avoid. This was the issue that I saw with np.place.
There was a problem hiding this comment.
Ah, good point. It's still not entirely clear to me why the current duck typing solution dies not suffice, though.
There was a problem hiding this comment.
Actually, I would suggest:
try:
put = a.put
except AttributeError:
raise TypeError
return put(...)
There was a problem hiding this comment.
@shoyer : Ah, fair enough. I do see lots of similar wrappings in this file, so I'll follow suit then. By the way, might you happen to know what that test_mem_insert is all about? Even with your suggestion, that test will fail, but I'm not sure what it is or what the purpose of that test was. I'm tempted to remove it, but it was located in the test_regression.py file, so I'm hesitant to do so until I have more information.
@mhvk : Thanks for your input as well!
|
@mhvk : I think I'll take the test out. I'm doing a complete revamp of |
|
Changes are in, and Travis + Appveyor are both happy. Should be good to merge if there is nothing else. |
There was a problem hiding this comment.
Could we use the same duck-typing solution as above, with the try/except clause?
There was a problem hiding this comment.
It feels little more awkward in this case because _insert is not an ndarray method. I suppose you could do something like the following:
try:
put = arr.put
except AttributeError:
raise TypeError(...)
return _insert(arr, mask, vals)
However, it seems like more contrived compared to the others since we're relying on a method that has nothing to do with what is being called.
There was a problem hiding this comment.
What error message do you get when place is not valid? What sort of duck typing does it currently support, if any?
It might be worth updating _insert instead.
There was a problem hiding this comment.
@shoyer : Sorry, I didn't answer the remainder of your question; I only had seen the "It might be worth" part on my phone when I was reading it. place doesn't actually do any duck-typing if you pass in a non-ndarray to it. Rather, it does nothing, which is why I added that isinstance check to the code. However, #7003 will embed that check into the C code, which is preferable. However, I wouldn't mind merging the isinstance check into the codebase FTTB.
|
It looks like we still need a unit test for the |
|
@shoyer : Oh, yes, of course! Let me do that quickly. |
|
@shoyer : Actually, quick question : where should that test go? It seems that there are no tests for the |
|
Looks like test_place is in test_function_base.py. On Wed, Jan 13, 2016 at 11:45 AM, gfyoung [email protected] wrote:
|
|
@shoyer : I'm referring to |
|
Looks like that is in test_multiarray.py. On Wed, Jan 13, 2016 at 11:52 AM, gfyoung [email protected] wrote:
|
|
It's for the array method itself, not |
|
Hmm. My search for np.put in test_* turns up nothing. So no wonder this was On Wed, Jan 13, 2016 at 11:54 AM, gfyoung [email protected] wrote:
|
|
Oh, okay! I'm relatively new as well (I started contributing last month), but I think I'm going to go for creating a new file called |
|
@shoyer : I'm going to create a new file |
np.put and np.place do something only when the first argument is an instance of np.ndarray. These changes will cause a TypeError to be thrown in either function should that requirement not be satisfied.
|
Test added and passing on both Travis and Appveyor. Should be good to merge if there is nothing else. |
|
OK, I'm pretty happy with this. Going to merge unless someone else raises objections.... |
|
I'm not 100% sure that |
DOC, MAINT: Enforce np.ndarray arg for np.put and np.place
|
Thanks @gfyoung! |
np.putandnp.placedo something only when the first argument is an instance ofnp.ndarray. These changes will cause aTypeErrorto be thrown in either function should that requirement not be satisfied, which would be more useful to see when debugging.