8000 DOC: describe the expansion of take and apply_along_axis in detail by eric-wieser · Pull Request #9946 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

DOC: describe the expansion of take and apply_along_axis in detail #9946

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 28, 2017
Merged

DOC: describe the expansion of take and apply_along_axis in detail #9946

merged 1 commit into from
Nov 28, 2017

Conversation

eric-wieser
Copy link
Member

Extracted from gh-8714

@eric-wieser
Copy link
Member Author
eric-wieser commented Nov 1, 2017 •
edited
Loading

Could alternative take a less pseudo-cody approach, and instead of

for each `i...`, `j...`, `k...` in turn (representing a series of indices)

out[i..., ..., k...] = a[i..., :, k...][indices]

use

for each `i0, i1, i2` in turn (each a tuple of indices)

out[i0 + np.s_[...] + i2] = a[i0 + np.s_[:] + i2][indices]

with a comment that i0 and i2 are tuples of indices

@eric-wieser eric-wieser mentioned this pull request Nov 1, 2017
Closed
charris
charris reviewed Nov 4, 2017
View reviewed changes
numpy/lib/shape_base.py Outdated
@@ -27,14 +27,24 @@ def apply_along_axis(func1d, axis, arr, *args, **kwargs):
Execute `func1d(a, *args)` where `func1d` operates on 1-D arrays and `a`
is a 1-D slice of `arr` along `axis`.

For each `i...`, `j...`, `k...` in turn (representing a series of indices),
where the number of indices in `i...` is equal to `axis`, this computes::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, clear as mud. Also, most uses of "( ... )" are better punctuated with commas.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your point about ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member
@ahaldane ahaldane Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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:"

Copy link
Member Author
@eric-wieser eric-wieser Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ahaldane
ahaldane reviewed Nov 20, 2017
View reviewed changes
numpy/core/fromnumeric.py Outdated
@@ -70,11 +70,21 @@ def take(a, indices, axis=None, out=None, mode='raise'):
using arrays); however, it can be easier to use if you need elements
along a given axis.
Copy link
Member
@ahaldane ahaldane Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,...]

@eric-wieser eric-wieser force-pushed the improve-take-docs branch 2 times, most recently from 15721dd to 3ed0a19 Compare November 21, 2017 09:34
@eric-wieser
8000 Copy link
Member Author

Thanks for the feedback @ahaldane, using ndindex is much clearer than inventing syntax. Updated.

I've left the (Nj...) in the parameter descriptions. alone, since I can't think of a better way to represent those.

@eric-wieser eric-wieser force-pushed the improve-take-docs branch 5 times, most recently from c43f74d to 7753b1e Compare November 21, 2017 09:52
eric-wieser
eric-wieser commented Nov 21, 2017
View reviewed changes
numpy/core/fromnumeric.py
for ii in ndindex(Ni):
for jj in ndindex(Nj):
for kk in ndindex(Nk):
out[ii + jj + kk] = a[ii + (indices[jj],) + kk]
Copy link
Member Author
@eric-wieser eric-wieser Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member
@ahaldane ahaldane Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, with some rephrasing

eric-wieser
eric-wieser commented Nov 21, 2017
View reviewed changes
numpy/core/fromnumeric.py
Ni, Nk = a.shape[:axis], a.shape[axis+1:]
for ii in ndindex(Ni):
for kk in ndindex(Nj):
out[ii + s_[...,] + kk] = a[ii + s_[:,] + kk][indices]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the above comment, could be:

out[(*ii, ..., *kk)] = a[(*ii, s_[:], *kk)]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a little less mental overhead for tuple-addition than for tuple-unpacking, especially for people new to python.

ahaldane
ahaldane reviewed Nov 21, 2017
View reviewed changes
numpy/lib/shape_base.py
Ni, Nk = a.shape[:axis], a.shape[axis+1:]
for ii in ndindex(Ni):
for kk in ndindex(Nk):
out[ii + s_[...,] + kk] = func1d(arr[ii + s_[:,] + kk])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am tempted to skip the first longer version, and only leave this one, which I think is clearer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eric-wieser eric-wieser force-pushed the improve-take-docs branch 2 times, most recently from ca2ec0a to 0a2ead6 Compare November 22, 2017 05:43
@eric-wieser
Copy link
Member Author

Updated with the a less general but more clear example in take, and moved the second generalization to notes

@ahaldane
Copy link
Member

LGTM, let's let it sit a little bit in case anyone else wants to chime in.

@eric-wieser
Copy link
Member Author

Let's get this in 1.14 so that I can refer to it whilst trying to explain #8714 for 1.15

@eric-wieser eric-wieser added this to the 1.14.0 release milestone Nov 28, 2017
@ahaldane
Copy link
Member

Thanks for the reminder. Here goes...

@ahaldane ahaldane merged commit ce140e8 into numpy:master Nov 28, 2017
@eric-wieser eric-wieser mentioned this pull request May 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
04 - Documentation component: numpy._core component: numpy.lib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric-wieser @ahaldane @charris
0