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

ENH: Add empty property to Index. #15270

Closed
wants to merge 1 commit into from

Conversation

ssanderson
Copy link
Contributor

@ssanderson ssanderson commented Jan 31, 2017

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.

@codecov-io
Copy link

codecov-io commented Jan 31, 2017

Codecov Report

Merging #15270 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            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
Impacted Files Coverage Δ
pandas/core/base.py 95.17% <100%> (+0.02%)
pandas/core/common.py 91.02% <ø> (-0.34%)

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 12f2c6a...bb0126f. Read the comment docs.

@ssanderson
Copy link
Contributor Author

Happy to open a separate issue for this to close if it's desired. Whatsnew entry is currently just pointing at this PR.

Copy link
Contributor

@jreback jreback left a 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

@@ -877,6 +877,10 @@ def _values(self):
""" the internal implementation """
return self.values

@property
def empty(self):
return not bool(self.size)
Copy link
Contributor

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

Copy link
Contributor Author

@ssanderson ssanderson Jan 31, 2017

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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)
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@jreback
Copy link
Contributor

jreback commented Jan 31, 2017

PR number is fine

though pls search for an open issue (might be one)

@ssanderson
Copy link
Contributor Author

Looks like this was actually already reported as #13207 and claims to be fixed in #13216, though that branch looks like it's had some rebasing troubles and no longer appears to have the change related to .empty.

@ssanderson
Copy link
Contributor Author

Whatsnew added and comment added in the tests.

@jorisvandenbossche
Copy link
Member

The other PR was linked to the wrong issue. It was indeed fixing something else (corrected that now)

@@ -877,6 +877,10 @@ def _values(self):
""" the internal implementation """
return self.values

@property
def empty(self):
return not self.values.size
Copy link
Member

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.

Copy link
Contributor

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

@jreback jreback added Indexing Related to indexing on series/frames, not to indexes themselves Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Jan 31, 2017
@jreback
Copy link
Contributor

jreback commented Jan 31, 2017

@ssanderson trivial change. pls ping when green.

@jreback jreback added this to the 0.20.0 milestone Jan 31, 2017
@ssanderson
Copy link
Contributor Author

@jreback updated to just use .size.

@jreback
Copy link
Contributor

jreback commented Jan 31, 2017

while you are at it, can you add .empty in doc/source/api.rst (for Index, Series, DataFrame, Panel...etc)

@@ -877,6 +877,10 @@ def _values(self):
""" the internal implementation """
return self.values

@property
def empty(self):
return not self.size
Copy link
Contributor

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.

Copy link
Contributor Author

@ssanderson ssanderson Feb 20, 2017

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.

Copy link
Contributor

@jreback jreback Feb 20, 2017

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.

@jreback
Copy link
Contributor

jreback commented Feb 16, 2017

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`.
@jreback jreback removed this from the 0.20.0 milestone Mar 23, 2017
@jreback
Copy link
Contributor

jreback commented Mar 23, 2017

can you update

@jreback jreback added this to the 0.20.0 milestone Mar 28, 2017
@jreback jreback closed this in ec84ae3 Mar 28, 2017
@jreback
Copy link
Contributor

jreback commented Mar 28, 2017

thanks!

mattip pushed a commit to mattip/pandas that referenced this pull request Apr 3, 2017
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 <ssanderson@quantopian.com>

This patch had conflicts when merged, resolved by
Committer: Jeff Reback <jeff@reback.net>

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Indexing Related to indexing on series/frames, not to indexes themselves Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Index doesn't have an .empty attribute
4 participants