DOC: describe the expansion of take and apply_along_axis in detail#9946
DOC: describe the expansion of take and apply_along_axis in detail#9946ahaldane merged 1 commit intonumpy:masterfrom
Conversation
|
Could alternative take a less pseudo-cody approach, and instead of use with a comment that |
numpy/lib/shape_base.py
Outdated
There was a problem hiding this comment.
Umm, clear as mud. Also, most uses of "( ... )" are better punctuated with commas.
There was a problem hiding this comment.
I don't understand your point about ...
There was a problem hiding this comment.
Chuck is on a quest against parenthetical comments :) (not ellispsis)
(rightly I think, replacing them with sentence-clauses/commas improved my doc commits in #9056)
There was a problem hiding this comment.
Maybe this would be better if you could write some short python pseudocode for what happens? Eg
for i,j,k in np.ndindex(src.shape[0], len(indices), src.shape[2]):
dst[i,j,k] = src[i,indices[j],k]There was a problem hiding this comment.
replace = lambda tup,i,v: tup[:i] + (v,) + tup[i+1:]
dst = np.empty(replace(src.shape, axis, len(indices)))
for dst_indx in np.ndindex(dst.shape):
src_indx = replace(dst_indx, axis, indices[dst_indx[axis]])
dst[dst_indx] = src[src_indx]I'm not sure it ended up at all clear...
I might prefer my last comment's version if you preface it with "For example, for a 3 dimensional array src, np.take(src, indices, axis=1) is equivalent to:"
There was a problem hiding this comment.
How about:
for ii in ndindex(a.shape[:axis]):
for jj in ndindex(indices.shape):
for kk in ndindex(a.shape[axis+1:]):
out[ii + jj + kk] = a[ii + (indices[jj],) + kk]or maybe
# ii, jj, and kk are tuples of indices
for ii, jj, kk in product(
ndindex(a.shape[:axis]),
ndindex(indices.shape),
ndindex(a.shape[axis+1:])):
out[ii + jj + kk] = a[ii + (indices[jj],) + kk]There was a problem hiding this comment.
hey, that's not too bad. I find it fairly legible.
Stepping back and squinting, I like the first one better, especially if you add the comment from the second.
numpy/core/fromnumeric.py
Outdated
There was a problem hiding this comment.
I'm also a little surprised this doesn't explicitly say somthing like " this is equivalent to a[:,:,:,indices,:,:,:] with indices placed in the axis axis". Or a[:,:,:,indices,...]
15721dd to
3ed0a19
Compare
|
Thanks for the feedback @ahaldane, using I've left the |
c43f74d to
7753b1e
Compare
numpy/core/fromnumeric.py
Outdated
There was a problem hiding this comment.
Alternatively:
out[(*ii, *jj, *kk)] = a[(*ii, indices[jj], *kk)]It's a shame the unpacking generalizations don't apply to tuples within indexing brackets
There was a problem hiding this comment.
Also, while we are using s_, another way to express take is
arr[(s_[:],)*axis + (indices, Ellipsis)]
which is essentially what my comment in the intro of the docstring was saying about a[:,:,:,indices,...].
What about adding something like this to the docstring? :
This function does the same thing as "fancy" indexing (indexing arrays
using arrays); however, it can be easier to use if you need elements
along a given axis and supports `out` and `mode` arguments.
That is, a call such as ``np.take(arr, indices, axis=3)`` is equivalent to
``arr[:,:,:,indices,...]``. More explicitly, `take` is equivalent to the
following use of `ndindex` which sets each of ``ii``, ``jj``, and ``kk``
to a tuple of indices::
There was a problem hiding this comment.
Added, with some rephrasing
numpy/core/fromnumeric.py
Outdated
There was a problem hiding this comment.
As in the above comment, could be:
out[(*ii, ..., *kk)] = a[(*ii, s_[:], *kk)]There was a problem hiding this comment.
I think there is a little less mental overhead for tuple-addition than for tuple-unpacking, especially for people new to python.
numpy/lib/shape_base.py
Outdated
There was a problem hiding this comment.
I am tempted to skip the first longer version, and only leave this one, which I think is clearer.
There was a problem hiding this comment.
The longer version is more explicit, and is easier to compare to take IMO. I'd be happy to switch the order of these though.
ca2ec0a to
0a2ead6
Compare
Extracted from numpygh-8714 [ci-skip]
0a2ead6 to
21ef138
Compare
|
Updated with the a less general but more clear example in |
|
LGTM, let's let it sit a little bit in case anyone else wants to chime in. |
|
Let's get this in 1.14 so that I can refer to it whilst trying to explain #8714 for 1.15 |
|
Thanks for the reminder. Here goes... |
Extracted from gh-8714