ENH: Add length attribute to Interval and IntervalIndex #18805
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18805 +/- ##
==========================================
+ Coverage 91.65% 91.67% +0.02%
==========================================
Files 154 154
Lines 51368 51373 +5
==========================================
+ Hits 47080 47095 +15
+ Misses 4288 4278 -10
Continue to review full report at Codecov.
|
pandas/core/indexes/interval.py
Outdated
| Return an Index with entries denoting the length of each Interval in | ||
| the IntervalIndex | ||
| """ | ||
| return self.right - self.left |
There was a problem hiding this comment.
Some intervals don't have a length, if values are categorical-like, e.g. letters (pd.Interval('b', 'e')).
I think maybe the length calculation should be wrapped in a try/except statement, with a helpful error message for when length cannot be calculated (e.g. "not possible to calculate length between 'b' and 'e'" in the above example.
There was a problem hiding this comment.
@topper-123 : I agree with this assessment. @jschendel : Do wrap the length property as such and add tests.
There was a problem hiding this comment.
Done. Should probably audit the existing code for more instances of this, as I imagine there are a few places where similar failures occur, e.g. Interval.mid.
There was a problem hiding this comment.
That's probably a good idea. Depend on how big that change is, you may want to leave that as a follow-up PR though.
There was a problem hiding this comment.
Yeah, planning to do it in a separate PR so I don't delay this one with unrelated work while I look for more instances. Cleaner than having the instances I immediately see go in here, and the others in a separate PR too.
jschendel
force-pushed
the
interval-length
branch
from
2e5cb98 to
ac711a3
Compare
|
can you make sure this is in api.rst (though IIRC we are listing these attributes directly in the class). |
jschendel
force-pushed
the
interval-length
branch
from
ac711a3 to
ee78449
Compare
pandas/_libs/interval.pyx
Outdated
| return self.right - self.left | ||
| except TypeError: | ||
| # length not defined for some types, e.g. string | ||
| msg = 'cannot compute length between {left} and {right}' |
There was a problem hiding this comment.
A nitpick perhaps: IMO the form msg = 'cannot compute length between {left!r} and {right!r}' (note the !r) is better, so variables get outputted as raw strings in error messages.
In general IMO variables in error messages should use !r, though this is not very common in pandas or in general. Does pandas have a policy/view on this?
jschendel
force-pushed
the
interval-length
branch
2 times, most recently
from
9e54733 to
c891a80
Compare
|
@jreback : added this to api.rst, and made minor changes to the |
jschendel
force-pushed
the
interval-length
branch
2 times, most recently
from
feda6a7 to
dd534b7
Compare
doc/source/whatsnew/v0.22.0.txt
Outdated
| @@ -141,6 +141,7 @@ Other Enhancements | |||
| - ``IntervalIndex.to_tuples()`` has gained the ``na_tuple`` parameter to control whether NA is returned as a tuple of NA, or NA itself (:issue:`18756`) | |||
| - ``Categorical.rename_categories``, ``CategoricalIndex.rename_categories`` and :attr:`Series.cat.rename_categories` | |||
| can now take a callable as their argument (:issue:`18862`) | |||
| - :class:`Interval` and :class:`IntervalIndex` have gained a ``length`` attribute (:issue:`18789`) | |||
There was a problem hiding this comment.
rebase on master. can you move to 0.23 (docs were renamed), prob easiest to just check this file from master and past in new one
jschendel
force-pushed
the
interval-length
branch
from
dd534b7 to
6a719ef
Compare
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff