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: Series.idxmax actually returns a label, not an index #16829

Closed
AllenDowney opened this issue Jul 5, 2017 · 12 comments
Closed

DOC: Series.idxmax actually returns a label, not an index #16829

AllenDowney opened this issue Jul 5, 2017 · 12 comments
Labels
Docs good first issue Indexing Related to indexing on series/frames, not to indexes themselves
Milestone

Comments

@AllenDowney
Copy link
Contributor

In https://github.com/pandas-dev/pandas/blob/master/pandas/core/series.py, the DocStrings for idxmax and idxmin are misleading (in my opinion). They say they return the index of the max/min, but they actually return the label (if I've got my Pandas lingo right).

Same with DataFrame.idxmax and DataFrame.idxmin.

Also, is there a reason Series provides argmax as a synonym for idxmax, and DataFrame does not?

One more thought: I find it surprising that idxmax and argmax do the same thing. I expected idxmax to return an index and argmax to return a label. I suppose it's too late to make that change.

@TomAugspurger
Copy link
Contributor

Agreed that changing to label would be clearer.

For the idxmax vs. argmax, I think that was accidentally broken once Series no longer inherited from ndarray, see #6214.
I think we could issue a FutureWarning in 0.21.0, saying to use .idxmax instead of .argmax, and then followup to change .argmax to always be positional.

@TomAugspurger TomAugspurger added Docs Indexing Related to indexing on series/frames, not to indexes themselves labels Jul 5, 2017
@TomAugspurger TomAugspurger added this to the Next Major Release milestone Jul 5, 2017
@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jul 5, 2017

Let's leave this issue for the documentation, and I'll open a new one for the argmax.

edit: #16830

@AllenDowney
Copy link
Contributor Author

AllenDowney commented Jul 5, 2017 via email

@iuliakhomenko
Copy link
Contributor

Hi! I will correct idxmax and idxmin documentation for both Series and DataFrame

@gfyoung
Copy link
Member

gfyoung commented Aug 23, 2017

@iuliakhomenko : Go for it! Thanks for volunteering!

@TomAugspurger
Copy link
Contributor

Hi @iuliakhomenko. I think the ongoing work in #16955 will make the changes here unnecessary, so this may not be a good issue to work on.

We have plenty of other issues to take on if you're interested in contributing though :) Let us know if you need help finding one.

@iuliakhomenko
Copy link
Contributor

@TomAugspurger ok, I will look into other issues. Thanks :)

@gfyoung
Copy link
Member

gfyoung commented Aug 23, 2017

I think the ongoing work in #16955 will make the changes here unnecessary

@TomAugspurger : Except that the work isn't going ATM ;)

I don't see why we shouldn't address this issue unless we definitely plan on pushing #16955 through, which judging from the lack of tagging, isn't going to happen for now.

@jorisvandenbossche
Copy link
Member

@gfyoung I think that PR should certainly go in, and is in fact almost ready. In the submitter doesn't respond the coming week(s), we can easily fix up and merge.

@gfyoung
Copy link
Member

gfyoung commented Aug 23, 2017

@jorisvandenbossche : Sounds good. I didn't know where that PR stood currently in the pipeline. Thanks for clarifying!

@jorisvandenbossche
Copy link
Member

Just my personal opinion :-) But I have tagged it now as such to make it clear

@jorisvandenbossche jorisvandenbossche modified the milestones: Next Major Release, 0.21.0 Mar 5, 2018
@jorisvandenbossche
Copy link
Member

This is fixed in the meantime by #16955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs good first issue Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Development

No branches or pull requests

6 participants