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: Improve the docstring of argsort. #19885

Closed
wants to merge 5 commits into from
Closed

Conversation

vsnupoudel
Copy link

Updated the docstring for argsort to make it hopefully more clearer

mattip and others added 2 commits November 9, 2020 09:09
add a new workflow
Updated the docstring for argsort to make it hopefully more clearer
@rkern
Copy link
Member

rkern commented Sep 16, 2021

The general docstring convention we (and most other Python packages) follow is to have the first paragraph be a one-line sentence. More detail like this should be in the following paragraphs. We already describe this in prose in the subsequent paragraph, so I would expand on this there.

When you include inline code examples, we use double-backticks to make sure that they get rendered with code fonts appropriately.

The returned indices can then be used as ``a_sorted = a[returned indices]`` which...

Instead of a[returned indices], I would explicitly use a[np.argsort(a)] to make it valid code. Adding an example in the Examples section that does this to demonstrate how these indices relate to the sorted array would be good. Currently, we just show the indices rather than make the explicit connection to the sorted array.

@melissawm
Copy link
Member

Not sure what's going on with CI but it's unrelated to this PR.

@charris charris changed the title Updated the docstring for argsort to make it clearer DOC: Improve the docstring of argsort. Sep 18, 2021
@charris
Copy link
Member

charris commented Sep 18, 2021

@vsnupoudel Please implement @rkern's suggestion. Also keep lines < 80 characters in length.

@mattip
Copy link
Member

mattip commented Oct 1, 2021

I am inclined to close this. The added information does not enhance what is already in the Returns clause.

vsnupoudel and others added 2 commits October 1, 2021 14:28
Accepting suggested changes

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
@rkern
Copy link
Member

rkern commented Oct 1, 2021

We get enough "bug reports" of argsort() not returning the "correct" results that I think we do need to improve the docstring somehow. Moving the clarifying example up from Returns to the main body of the docstring is probably the right call.

@vsnupoudel As mentioned, code snippets (and references to variables in those snippets) need to be quoted with double-backticks for proper formatting. See how we do it in the Returns section.

@vsnupoudel
Copy link
Author

And an example right after the one liner at the top would be nice. Maybe I wil modify the first example as well.

@rkern
Copy link
Member

rkern commented Oct 1, 2021

By the way, by "clarifying example", I just meant the inline a_sorted = a[np.argsort(a)], which I think is sufficient to give the gist without showing any specific output. I think we try to keep ">>>" examples down in Examples. But maybe that's just a rule in my head. I do think a new example which does show the result of a[np.argsort(a)] down in Examples would help, too.

These were the use cases that I searched ´´argmax()´´ like function for.
Index of the 4th largest element in an unsorted 1D array.

>>> x = np.array(1, 2, 0, 3, 4, 5])
>>> np.argsort(x)[-4]
Copy link
Member

Choose a reason for hiding this comment

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

The point of these examples is to show the output. I would just show the result of x[np.argsort(x)] instead of these two examples.

@charris charris changed the base branch from build_test to main November 3, 2021 04:56
@charris
Copy link
Member

charris commented Nov 3, 2021

This PR was against the wrong branch (build_test), I changed it to main, but maybe it should be closed..

@melissawm
Copy link
Member

@vsnupoudel gentle ping, do you have plans to update this PR?

@vsnupoudel
Copy link
Author

vsnupoudel commented Feb 10, 2022

@vsnupoudel gentle ping, do you have plans to update this PR?

Hi there,
Not trying to be rude, but it feels like there is a lot of red tape, just to update a few sentences and examples.
Rules look more like opinions than actual rules.
Having said that, this pull request can be ignored. I was just trying to address a confusion I personally felt while reading it myself.
Sincerely,
Bishnu

@vsnupoudel vsnupoudel closed this Feb 10, 2022
@vsnupoudel vsnupoudel deleted the patch-1 branch February 10, 2022 05:10
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.

None yet

5 participants