/pandas

ENH: Add empty property to Index. #15270

Closed
wants to merge 1 commit into
from
+11 −0

Conversation

Reviewers

@jreback jreback

@jorisvandenbossche jorisvandenbossche

Assignees
No one assigned
Labels
Indexing Reshaping
Projects
None yet
Milestone
  0.20.0
4 participants
@ssanderson @codecov-io @jreback @jorisvandenbossche
@ssanderson
Contributor

ssanderson commented Jan 31, 2017 • edited

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.

  • closes #13207
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry
@codecov-io

codecov-io commented Jan 31, 2017 • edited

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
Contributor

ssanderson commented Jan 31, 2017

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

jreback requested changes Jan 31, 2017
View changes

pls add s whatsnew note

pandas/core/base.py
@@ -877,6 +877,10 @@ def _values(self):
""" the internal implementation """
return self.values
+ @property
+ def empty(self):
+ return not bool(self.size)
@jreback

jreback Jan 31, 2017

Contributor

u can prob remove the one from series

@ssanderson

ssanderson Jan 31, 2017 • edited

Contributor

It looks like Series is inheriting its current implementation from NDFrame, but this one will win because IndexOpsMixin appears first in its bases.

@ssanderson

ssanderson Jan 31, 2017

Contributor

Do you think it's worth trying to push the implementation in NDFrame elsewhere?

@jreback

jreback Jan 31, 2017

Contributor

i don't think that impl with work here but u can try

@ssanderson

ssanderson Jan 31, 2017

Contributor

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
+
+ def test_empty(self):
+ index = self.create_index()
+ self.assertFalse(index.empty)
@jreback

jreback Jan 31, 2017

Contributor

add this pr number as a xokment

@jreback

jreback Jan 31, 2017

Contributor

comment

@ssanderson

ssanderson Jan 31, 2017

Contributor

Updated.

@jreback
Contributor

jreback commented Jan 31, 2017

PR number is fine

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

ssanderson commented Jan 31, 2017

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
Contributor

ssanderson commented Jan 31, 2017

Whatsnew added and comment added in the tests.
@jorisvandenbossche
Owner

jorisvandenbossche commented Jan 31, 2017

The other PR was linked to the wrong issue. It was indeed fixing something else (corrected that now)
pandas/core/base.py
@@ -877,6 +877,10 @@ def _values(self):
""" the internal implementation """
return self.values
+ @property
+ def empty(self):
+ return not self.values.size
@jorisvandenbossche

jorisvandenbossche Jan 31, 2017

Owner

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.

@jreback

jreback Jan 31, 2017

Contributor

I agree this should directly use .size

@jreback jreback added Indexing Reshaping labels Jan 31, 2017

@jreback
Contributor

jreback commented Jan 31, 2017

@ssanderson trivial change. pls ping when green.

jreback added this to the 0.20.0 milestone Jan 31, 2017

@ssanderson
Contributor

ssanderson commented Jan 31, 2017

@jreback updated to just use .size.
@jreback
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)
pandas/core/base.py
@@ -877,6 +877,10 @@ def _values(self):
""" the internal implementation """
return self.values
+ @property
+ def empty(self):
+ return not self.size
@jreback

jreback Jan 31, 2017

Contributor

actually this might just work for all NDFrame.

can you also move the doc-string to here.

@ssanderson

ssanderson Feb 20, 2017 • edited

Contributor

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.

@jreback

jreback Feb 20, 2017 • edited

Contributor

you can easily fix the doc-strings by using the _shared_docs. ok with having one for Index and one in generic instead.

@jreback
Contributor

jreback commented Feb 16, 2017

can you rebase / update ....so close!
@ssanderson ssanderson 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`.

All checks have passed

2 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
bb0126f

jreback removed this from the 0.20.0 milestone Mar 23, 2017

@jreback
Contributor

jreback commented Mar 23, 2017

can you update

jreback added this to the 0.20.0 milestone Mar 28, 2017

jreback closed this in ec84ae3 Mar 28, 2017

@jreback
Contributor

jreback commented Mar 28, 2017

thanks!

@mattip mattip added a commit to mattip/pandas that referenced this pull request Apr 3, 2017

@ssanderson @mattip ssanderson + mattip 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`.

Author: Scott Sanderson <ssanderson@quantopian.com>

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

closes #13207
Closes #15270 from ssanderson/add-empty-to-index and squashes the following commits:

bb0126f [Scott Sanderson] ENH: Add empty property to Index.
 0fd2cbb

@linebp linebp added a commit to linebp/pandas that referenced this pull request Apr 17, 2017

@ssanderson @linebp ssanderson + linebp 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`.

Author: Scott Sanderson <ssanderson@quantopian.com>

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

closes #13207
Closes #15270 from ssanderson/add-empty-to-index and squashes the following commits:

bb0126f [Scott Sanderson] ENH: Add empty property to Index.
 86f80cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment