DOC: Add note to nonzero docstring.
#13551
Conversation
numpy/core/fromnumeric.py
Outdated
| @@ -1761,6 +1761,8 @@ def nonzero(a): | |||
| """ | |||
| Return the indices of the elements that are non-zero. | |||
|
|
|||
| NOTE - In place of using x[y.nonzero()], it is recommended to use x[y.astype(bool)]. | |||
|
|
|||
There was a problem hiding this comment.
Please move this to after the Return section as
Notes
-----
To obtain the non-zero values, it is recommended to use ``x[y.astype(bool)]`` which
will correctly handle 0-d arrays.
and refactor the paragraph at 1766 to better express the idea that nonzero is the wrong tool for directly indexing the array.
If a line goes over 80 characters, break it up.
There was a problem hiding this comment.
the text "The corresponding non-zero ..." on line 1767 above needs rephrasing or removal.
nonzero docstring.
numpy/core/fromnumeric.py
Outdated
| Notes | ||
| ----- | ||
| To obtain the non-zero values, it is recommended to use ``x[y.astype(bool)]`` | ||
| which will correctly handle 0-d arrays. |
There was a problem hiding this comment.
The description you use here makes x vs y look like a typo. It would be good to include the form you're recommending against here too.
Also, worth commenting that in most cases y is already a boolean array, so as type is unnecessary
There was a problem hiding this comment.
@abhinavsagar could you follow up on this, and the other comments? I think it would be good to keep the old comment about the values and move it here, as well as mentioning the astype is not necessary.
The story would be that you can get the values using the old statement (with the exception of 0d), but if you only need the values, you should prefer the other statement.
There was a problem hiding this comment.
@seberg Done the required changes.
|
I'm not a member of the NumPy project, so feel free to take my review with a grain of salt if you like. But I am the one who opened issue #13522 which you say this fixes. I think (in addition to the note you've added here), it should be mentioned that |
numpy/core/fromnumeric.py
Outdated
| values can be obtained with:: | ||
|
|
||
There was a problem hiding this comment.
Nit: let's not add random whitespace to the patch
There was a problem hiding this comment.
Sorry, I was changing from my phone.
Given we're leaning towards a deperecation there, it might not be worth documenting |
|
@eric-wieser Done. Is it ready to be merged now ? |
numpy/core/fromnumeric.py
Outdated
| ----- | ||
| To obtain the non-zero values, it is recommended to use ``x[y.astype(bool)]`` | ||
| which will correctly handle 0-d arrays. In most cases y is already a boolean | ||
| array, so astype is not necessary. |
There was a problem hiding this comment.
Shouldn't this be x[x.astype(bool)] without the y?
There was a problem hiding this comment.
Now the y in the second line is wrong. Also since we now are using x, the sentence beginning "In most cases ..." since we don't know anything about x. Probably should drop that.
There was a problem hiding this comment.
Done. It seems like we came back from where we started.
|
Thanks @abhinavsagar |
Document problems with using
x.nonzerofor indexing, i.e., never use x[y.nonzero()], use x[y.astype(bool)] instead.Fixes #13522