Skip to content

DOC: clarify idxmax behaviour issue #18206 #18209

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 4 commits into from
Nov 14, 2017
Merged

DOC: clarify idxmax behaviour issue #18206 #18209

merged 4 commits into from
Nov 14, 2017

Conversation

ghasemnaddaf
Copy link
Contributor

@ghasemnaddaf ghasemnaddaf commented Nov 10, 2017 •
edited
Loading

@ghasemnaddaf ghasemnaddaf mentioned this pull request Nov 10, 2017
Closed
@ghasemnaddaf ghasemnaddaf mentioned this pull request Nov 10, 2017
Closed
ghasemnaddaf
ghasemnaddaf commented Nov 10, 2017
View reviewed changes
pandas/core/indexes/multi.py Outdated
@@ -811,7 +811,9 @@ def duplicated(self, keep='first'):

@Appender(ibase._index_shared_docs['fillna'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this line be removed?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. That appends additional documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, but I see #18018. In that case, leave this alone for this PR. Make the change in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved fillna changes to PR #18269 .

@gfyoung gfyoung added the Docs label Nov 10, 2017
@codecov
Copy link

codecov bot commented Nov 10, 2017 •
edited
Loading

Codecov Report

Merging #18209 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18209      +/-   ##
==========================================
- Coverage    91.4%   91.38%   -0.02%     
==========================================
  Files         163      164       +1     
  Lines       50064    49883     -181     
==========================================
- Hits        45759    45586     -173     
+ Misses       4305     4297       -8
Flag Coverage Δ
#multiple 89.19% <ø> (ø) ⬆️
#single 39.42% <ø> (-1%) ⬇️
Impacted Files Coverage Δ
pandas/core/series.py 95% <ø> (ø) ⬆️
pandas/core/frame.py 97.8% <ø> (-0.1%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/io/clipboard/clipboards.py 24.05% <0%> (-2.54%) ⬇️
pandas/tseries/frequencies.py 94.09% <0%> (-1.92%) ⬇️
pandas/core/categorical.py 95.75% <0%> (-0.05%) ⬇️
pandas/core/indexes/timedeltas.py 91.14% <0%> (-0.04%) ⬇️
pandas/core/dtypes/concat.py 99.13% <0%> (-0.02%) ⬇️
pandas/core/indexes/multi.py 96.39% <0%> (-0.01%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca737ac...6cc1f05. Read the comment docs.

jorisvandenbossche
jorisvandenbossche reviewed Nov 10, 2017
View reviewed changes
pandas/core/indexes/multi.py Outdated
@@ -811,7 +811,9 @@ def duplicated(self, keep='first'):

@Appender(ibase._index_shared_docs['fillna'])
def fillna(self, value=None, downcast=None):
# isna is not implemented for MultiIndex
"""
isna is not implemented for MultiIndex
Copy link
Member

Choose a reason for hiding this comment

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

isna -> fillna

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorisvandenbossche please see PR #18269 for changes related to fillna.

jorisvandenbossche
jorisvandenbossche reviewed Nov 10, 2017
View reviewed changes
pandas/core/series.py Outdated
@@ -1336,7 +1336,8 @@ def idxmax(self, axis=None, skipna=True, *args, **kwargs):
Parameters
----------
skipna : boolean, default True
Exclude NA/null values
Exclude NA/null values. If an entire row/column is NA, the result
Copy link
Member

Choose a reason for hiding this comment

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

"an entire row/column" -> "the entire Series"

@jorisvandenbossche
Copy link
Member

@ghasemnaddaf Thanks! Could you also add a 'Raises' section in the idxmin/idxmax docstring to say that it raises a ValueError when for empty rows/columns ?

@ghasemnaddaf
Copy link
Contributor Author

added Raises section

@jorisvandenbossche jorisvandenbossche added this to the 0.21.1 milestone Nov 14, 2017
@jorisvandenbossche jorisvandenbossche merged commit 63e8527 into pandas-dev:master Nov 14, 2017
@jorisvandenbossche
Copy link
Member

@ghasemnaddaf Thanks!

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request Dec 8, 2017
@ghasemnaddaf @TomAugspurger
DOC: clarify idxmax behaviour issue pandas-dev#18206 (pandas-dev#18209)
89b3c91 
(cherry picked from commit 63e8527)
TomAugspurger pushed a commit that referenced this pull request Dec 11, 2017
@ghasemnaddaf @TomAugspurger
DOC: clarify idxmax behaviour issue #18206 (#18209)
c0e4acd 
(cherry picked from commit 63e8527)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfyoung gfyoung gfyoung left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
0.21.1
Development

Successfully merging this pull request may close these issues.

DataFrame.idxmax returns NaN if all entries are NaN, contradicts idxmax documentation
3 participants
@ghasemnaddaf @jorisvandenbossche @gfyoung