-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
|
|
||
| @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.
|
|
||
| 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
|
|
||
| @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 |
|
|
||
| @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