Skip to content
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.

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

DOC: Add take_along_axis to the see also section in argmin, argmax etc. #14799

Merged
merged 18 commits into from
Nov 4, 2019

Conversation

mproszewska
Copy link
Contributor

@mproszewska mproszewska commented Oct 29, 2019

This closes #6078.

@@ -797,6 +797,7 @@ def argpartition(a, kth, axis=-1, kind='introselect', order=None):
partition : Describes partition algorithms used.
ndarray.partition : Inplace partition.
argsort : Full indirect sort
take_along_axis : Take values from the input array by matching 1d index and data slices.
Copy link
Member

@eric-wieser eric-wieser Oct 29, 2019

Choose a reason for hiding this comment

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

Let's see how another maintainer feels about this first, but perhaps:

Suggested change
take_along_axis : Take values from the input array by matching 1d index and data slices.
take_along_axis : Apply ``index_array`` from argpartition to an array as if by calling partition

And same for the below,

    take_along_axis : Apply ``index_array`` from argsort to an array as if by calling sort
    take_along_axis : Apply ``np.expand_dims(index_array, axis)`` from argmin to an array as if by calling min
    take_along_axis : Apply ``np.expand_dims(index_array, axis)`` from argmax to an array as if by calling max

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably those use cases are worth mentioning, but I wasn't sure if the "See also" section is a right place for that. e.g. argsort and argpartition refer to take_along_axis in "Returns". Perhaps it's better to just add an example.

Copy link
Member

Choose a reason for hiding this comment

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

An example for each of these showing how to use take_along_axis would be great, if you're willing to spend the time. If not, we can merge this as is, and leave the issue open for someone else to add the examples.

Copy link
Member

Choose a reason for hiding this comment

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

"See Also" seems appropriate here.

@@ -1223,6 +1239,13 @@ def argmin(a, axis=None, out=None):
>>> np.argmin(b) # Only the first occurrence is returned.
0

>>> x = np.array([[4,2,3],[1,0,3]])
>>> np.take_along_axis(x, np.expand_dims(np.argmin(x, axis=-1), axis=-1), axis=-1)
Copy link
Member

@eric-wieser eric-wieser Nov 1, 2019

Choose a reason for hiding this comment

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

Possibly worth splitting these examples in two, either as:

Suggested change
>>> np.take_along_axis(x, np.expand_dims(np.argmin(x, axis=-1), axis=-1), axis=-1)
>>> index_array = np.argmin(x, axis=-1)
>>> np.take_along_axis(x, np.expand_dims(index_array, axis=-1), axis=-1)

or

Suggested change
>>> np.take_along_axis(x, np.expand_dims(np.argmin(x, axis=-1), axis=-1), axis=-1)
>>> index_array = np.expand_dims(np.argmin(x, axis=-1), axis=-1) # emulate keepdims=True
>>> np.take_along_axis(x, index_array, axis=-1)

mproszewska and others added 3 commits November 1, 2019 01:08
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
mproszewska and others added 9 commits November 2, 2019 01:31
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Co-Authored-By: Eric Wieser <wieser.eric@gmail.com>
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks good, will let someone else merge since I suggested a little too much of the wording to be impartial.

>>> np.take_along_axis(x, np.expand_dims(index_array, axis=-1), axis=-1) # same as np.min(x, axis=-1, keepdims=True)
array([[2],
[0]])
>>> np.take_along_axis(x, np.expand_dims(index_array, axis=-1), axis=-1).squeeze(axis=-1) # same as np.max(x, axis=-1)
Copy link
Member

@eric-wieser eric-wieser Nov 3, 2019

Choose a reason for hiding this comment

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

Nit: pep8 wants two spaces before #, same on other lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was looking for that bug

[3]])
>>> np.take_along_axis(x, np.expand_dims(index_array, axis=-1), axis=-1).squeeze(axis=-1) # same as np.max(x, axis=-1)
array([4, 3])

Copy link
Member

Choose a reason for hiding this comment

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

These lines are quite long. Maybe move the comment to stand alone in the line above the code

@mattip
Copy link
Member

mattip commented Nov 3, 2019

A small nit about line length, but otherwise this looks good

@mattip mattip merged commit 7b8513f into numpy:master Nov 4, 2019
@mattip
Copy link
Member

mattip commented Nov 4, 2019

Thanks @mproszewska

@mproszewska mproszewska deleted the doc-take-along-axis branch November 18, 2019 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: argmin, argmax, argort, etc. should link to np.take_along_axis in their documentation
3 participants