BUG: Perform i8 conversion for datetimelike IntervalTree queries by jschendel · Pull Request #22988 · pandas-dev/pandas (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Could you also add tests for Periods as well? I think they also need i8 conversion (though possibly less commonly used).
We currently do not allow an IntervalIndex to have Period endpoints, so I don't think tests are necessary:
In [2]: pd.IntervalIndex.from_breaks(pd.period_range('2018Q1', freq='Q', periods=3))
ValueError: Period dtypes are not supported, use a PeriodIndex instead
Interestingly enough, it looks like an Interval can have Period endpoints, which seems like a bug:
In [3]: pd.Interval(pd.Period('2018Q1', freq='Q'), pd.Period('2018Q3', freq='Q')) Out[3]: Interval(Period('2018Q1', 'Q-DEC'), Period('2018Q3', 'Q-DEC'), closed='right')
Gotcha. Are there plans for IntervalIndex to support Period? That might get tricky since Periods themselves are conceptually 1 label for an interval of time.
Are there plans for IntervalIndex to support Period?
I'm not aware of any such plans. I don't think the same type algorithms would apply; a Period is basically a special case of an Interval, so you'd essentially have intervals with interval endpoints (e.g. Period('2018Q1') is conceptually Interval('2018-01-01', '2018-04-01', closed='left') with some additional special properties.). Haven't fully thought it through though.
There is an open issue to have Period/PeriodIndex inherit from Interval/IntervalIndex though, allowing one to use any of the interval methods on periods. Not quite as straight forward as it might initially seem though, as we allow a Period to be outside the bounds of Timestamp, so just using Timestamp endpoints for the Interval representation wouldn't cut it.
| return key |
|---|
| def _maybe_convert_i8(self, key): |
| if isinstance(key, Interval): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you document this a bit, and can you simplify at all?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Look better?
@mroeschke as indicated above, a Period doesn't make sense as an element of an IntervalIndex, though I don't think we actually preclude it, maybe we should. However implemented Period as a sub-class of Intevral and PeriodIndex as a sub-class of IntervalIndex, does make sense (though may not be straightforward to actually do this).
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request
