-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: Add empty property to Index. #15270
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15270 +/- ##
==========================================
- Coverage 90.37% 90.37% -0.01%
==========================================
Files 135 135
Lines 49466 49468 +2
==========================================
+ Hits 44705 44706 +1
- Misses 4761 4762 +1
Continue to review full report at Codecov.
|
0b2826f
to
3e93ff0
Compare
Happy to open a separate issue for this to close if it's desired. Whatsnew entry is currently just pointing at this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls add s whatsnew note
pandas/core/base.py
Outdated
@@ -877,6 +877,10 @@ def _values(self): | |||
""" the internal implementation """ | |||
return self.values | |||
|
|||
@property | |||
def empty(self): | |||
return not bool(self.size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u can prob remove the one from series
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Series is inheriting its current implementation from NDFrame
, but this one will win because IndexOpsMixin
appears first in its bases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's worth trying to push the implementation in NDFrame
elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think that impl with work here but u can try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I didn't mean to imply that the NDFrame impl worked here, I meant that we could push the NDFrame impl into a subclass (e.g. into an intermediate class between NDFrame
and DataFrame
/Panel
). I doubt that's worth the added complexity though.
pandas/tests/indexes/common.py
Outdated
|
||
def test_empty(self): | ||
index = self.create_index() | ||
self.assertFalse(index.empty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this pr number as a xokment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
3e93ff0
to
66769f4
Compare
PR number is fine though pls search for an open issue (might be one) |
66769f4
to
76d6beb
Compare
Whatsnew added and comment added in the tests. |
76d6beb
to
9ee52a4
Compare
The other PR was linked to the wrong issue. It was indeed fixing something else (corrected that now) |
pandas/core/base.py
Outdated
@@ -877,6 +877,10 @@ def _values(self): | |||
""" the internal implementation """ | |||
return self.values | |||
|
|||
@property | |||
def empty(self): | |||
return not self.values.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also use self.size
instead?
Index.size
uses the _values
instead of values
. Didn't follow that recently, but I think the values
get converted to objects for PeriodIndex, giving an unnecessary slowdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this should directly use .size
@ssanderson trivial change. pls ping when green. |
9ee52a4
to
f329977
Compare
@jreback updated to just use |
while you are at it, can you add |
@@ -877,6 +877,10 @@ def _values(self): | |||
""" the internal implementation """ | |||
return self.values | |||
|
|||
@property | |||
def empty(self): | |||
return not self.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this might just work for all NDFrame.
can you also move the doc-string to here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the current docstring on NDFrame would make sense if moved here, since it makes references to NDFrame
, and it has examples geared toward DataFrame
, both of which would be confusing if they showed up on a property of Index
.
Coming back to this with fresh eyes, I'm now inclined to just put empty
on the base Index
class and give it an Index-specific docstring. The root issue here is that there are two places from which Series
could be getting it's implementation of empty
: NDFrame
, which provides shared implementations for Series/DataFrame/Panel, and IndexOpsMixin
, which provides shared implementations for Index
and Series
. In the case of empty
(and, in general, any property that belongs on both containers and indexes) I think putting the implementation on IndexOpsMixin
ends up being more confusing than helpful, because it's no longer clear which implementation will end up getting used for Series
. Does that seem reasonable?
Separately, I could replace the current implementation of NDFrame.empty
with this if you think it's preferable, though I'm not sure it's a worthwhile improvement by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can easily fix the doc-strings by using the _shared_docs. ok with having one for Index and one in generic instead.
can you rebase / update ....so close! |
Previously, attempting to evaluate an Index in a boolean context prints an error message listing various alternatives, one of which is `.empty`, which was not actually implemented on `Index`.
f329977
to
bb0126f
Compare
can you update |
thanks! |
Previously, attempting to evaluate an Index in a boolean context prints an error message listing various alternatives, one of which is `.empty`, which was not actually implemented on `Index`. Author: Scott Sanderson <[email protected]> This patch had conflicts when merged, resolved by Committer: Jeff Reback <[email protected]> closes pandas-dev#13207 Closes pandas-dev#15270 from ssanderson/add-empty-to-index and squashes the following commits: bb0126f [Scott Sanderson] ENH: Add empty property to Index.
Previously, attempting to evaluate an Index in a boolean context prints
an error message listing various alternatives, one of which is
.empty
,which was not actually implemented on
Index
.git diff upstream/master | flake8 --diff