Skip to content

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

Merged
mattip merged 18 commits into
numpy:masterfrom
mproszewska:doc-take-along-axis
Nov 4, 2019
Merged

mattip merged 18 commits into
numpy:masterfrom
mproszewska:doc-take-along-axis

Conversation

@mproszewska

@mproszewska mproszewska commented Oct 29, 2019 •

Copy link
Copy Markdown
Contributor

This closes #6078.

Comment thread numpy/core/fromnumeric.py Outdated
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.

@eric-wieser eric-wieser Oct 29, 2019 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread numpy/core/fromnumeric.py Outdated
Comment thread numpy/core/fromnumeric.py Outdated
Comment thread numpy/core/fromnumeric.py Outdated
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)

@eric-wieser eric-wieser Nov 1, 2019 •

Copy link
Copy Markdown
Member

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)

Comment thread numpy/core/fromnumeric.py Outdated
Comment thread numpy/core/fromnumeric.py Outdated
Comment thread numpy/core/fromnumeric.py Outdated
Comment thread numpy/core/fromnumeric.py Outdated

@eric-wieser eric-wieser left a comment

Copy link
Copy Markdown
Member

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.

Comment thread numpy/core/fromnumeric.py Outdated
>>> 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)

@eric-wieser eric-wieser Nov 3, 2019 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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

Comment thread numpy/core/fromnumeric.py
[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
Copy Markdown
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

mattip commented Nov 3, 2019

Copy link
Copy Markdown
Member

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

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

mattip commented Nov 4, 2019

Copy link
Copy Markdown
Member

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