ENH: implement DatetimeLikeArray
State: closed

The medium-term goal: refactor out of `DatetimeIndexOpsMixin`/`DatetimeIndex`/`TimedeltaIndex`/`PeriodIndex` the bare minimum subset of functionality to implement arithmetic+comparisons for `DatetimeArray`/`TimedeltaArray`/`PeriodArray`.  This PR does _not_ do that.

What it _does_ do is refactor out the subset of those methods that can be transplanted directly into the Array classes (i.e. cut/paste).

On its own this PR is not very useful, so think of it as a Proof of Concept/discussion piece.

cc @TomAugspurger since this is a precursor to getting a "real" PeriodArray.

Labels:
name: Enhancement
name: Internals
description: Related to non-user accessible pandas implementation
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=h1) Report
> Merging [#19902](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/ad76ffcca0d92c3885c279c80701c2f4a3f3f177?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `96.85%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/19902/graphs/tree.svg?token=eZ4WkYLtcO&src=pr&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #19902      +/-   ##
==========================================
+ Coverage    91.9%    91.9%   +<.01%     
==========================================
  Files         154      158       +4     
  Lines       49659    49701      +42     
==========================================
+ Hits        45640    45680      +40     
- Misses       4019     4021       +2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.28% <96.85%> (ø)` | :arrow_up: |
| #single | `41.94% <74.84%> (+0.04%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/timedelta.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YS5weQ==) | `100% <100%> (ø)` | |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.19% <100%> (-0.06%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.48% <100%> (-0.18%)` | :arrow_down: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `92.58% <100%> (-0.1%)` | :arrow_down: |
| [pandas/core/indexes/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZWxpa2UucHk=) | `96.72% <100%> (+0.24%)` | :arrow_up: |
| [pandas/core/arrays/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL19faW5pdF9fLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `100% <100%> (ø)` | |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `100% <100%> (ø)` | |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `92.75% <92.75%> (ø)` | |
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `94.96% <0%> (-0.01%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/pandas-dev/pandas/pull/19902/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=footer). Last update [ad76ffc...a684c2d](https://codecov.io/gh/pandas-dev/pandas/pull/19902?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Moved the new classes to core.arrays directory.  The set of cut/paste-able methods will be expanded when #19912 and #19903 go through.
Now that #19800 is in, the follow-up to this can include all of the comparison method (the Index ops will need a ~2 line wrapper around the array ops)
Thoughts on where to go with this?  The steps after this are going to require a lot of work to carefully port the appropriate tests, so I'd like to keep slow-and-steady momentum.
> can you sketch out your next few steps here?

This is ~everything that can be cut/paste directly.  The next step I have in mind is a handful of methods that need a 1-line wrapping in the Index classes.  e.g. comparison methods are non-Index-specific right up until the last line when they wrap an ndarray in an Index.  Following that are the arithmetic methods, for which the wrapping is less trivial.  Does that answer the question?

> Can you edit the OP in #19696?

Sure.

BTW let me know if time is an issue on this or other PRs.  I've been distracted for the last couple of weeks with a bugfix-fork of statsmodels that's left a bunch of stuff here on hiatus.

> BTW let me know if time is an issue on this or other PRs.

It's starting to be. I think we want to do a release candidate in the next couple weeks. It'd be nice to have as much of the EA stuff done as possible. My plan is for groupby to be the last bit of API that we ensure works, and then pick up moving our other extension types over to the new interface. If you're able to take on any of that it'd be great.

> bugfix-fork of statsmodels

That's unfortunate, but understandable :/ I'm hoping to push on a statsmodels release sometime shortly after pandas 0.23.0.
Making suggested changes now.  Will push shortly.

re `arrays.__init__`, any thoughts about defining `__all__` and removing the `# noqa`s?  Not a big deal, just a slight preference.

re statsmodels: see [sm2](github.com/jbrockmendel/sm2).  The vague hope is that it gets enough community traction to convince jpkt to take technical debt seriously, at which point fixes can be upstreamed and it can become unnecessary.
> re arrays.__init__, any thoughts about defining __all__ and removing the # noqas? Not a big deal, just a slight preference.

No preference at all.

@jorisvandenbossche / @jreback this LGTM. Any concerns?
How do I have to see this PR? 
Is this is a first *actual* stab at implementing the array types for datetime/period/timedelta? Or more a reorganisation of code?

Because if it is the first, I probably have a bunch of comments on what we exactly want to put in the array classes. And also, if that is the case, I am not sure we want the Index to subclass them? I thought we would rather go for composition?

Reorganization. `DatetimeLikeArray` isn't an ExtensionArray yet.

> And also, if that is the case, I am not sure we want the Index to subclass them? I thought we would rather go for composition?

I've been going back and forth on which approach is best here. I'm slightly coming around to the idea of subclassing, but haven't 100% settled yet. I think that the changes here are going to be helpful either way, correct @jbrockmendel? At some point we'll either make DatetimeLikeArray inherit from ExtensionArray, or the Index classes will change to compose it.
> Reorganization. DatetimeLikeArray isn't an ExtensionArray yet.

Yes, but it is a bit confusing this PR already adds the names "DatetimeArray" etc (not only the "like") as well without them being "arrays" in the sense of our array interface. 
But as long as we agree and know this is only temporary, that's fine.
One other thing worth discussing: do we want to do first-class support for Period & Interval, etc. for pandas 0.23? Let's discuss in https://github.com/pandas-dev/pandas/issues/19696
> How do I have to see this PR?  Is this is a first actual stab at implementing the array types for datetime/period/timedelta? Or more a reorganisation of code?

Is "actual PR that reorganizes code" an option?  This is the first in a sequence of at least three.  This PR moves methods from the index classes verbatim, largely to simplify the diffs in upcoming passes.  At the end of the ~3 PR process, _then_ we'll be ready to address

- possibly subclassing some type of `PandasBaseArray`
- values/_values/_ndarray_values/data/_data/...
- inheritance vs composition
@TomAugspurger thoughts on where to go with this?
I am conceptually ok with doing this as its just moving things around now, setting the stage for EA. The issue is that I think we should do this post 0.23.0, or delay it significantly (not great).
OK, I'll keep rebasing occasionally to keep this merge-ready.  Not in an especially big hurry, though it'd be nice to get started on the next step.
Given discussion in https://github.com/pandas-dev/pandas/issues/19696 and https://github.com/pandas-dev/pandas/pull/20611, I think we will rather go with composition than subclassing. In that case, I am not sure I am in favor of the mixins you are adding in this PR.
> Given discussion in #19696 and #20611, I think we will rather go with composition than subclassing. In that case, I am not sure I am in favor of the mixins you are adding in this PR.

I think you ought to re-read my comments.

``Index`` *must* follow the EA protocol otherwise we have tons of duplicated code. This does NOT preclude have a composed internal implementation of an EA Array itself. In fact this is a very nice way to do things.

This is my point of using mixins. One of the primary goals is to simplify the internals of pandas itself by using a standardized protocol. My continued pushback on adding new code is for exactly this reason. The implementors of the current internal EA's (categorical, proposed intervals), need to refactor internal code. Otherwise *even* more technical debt is going to happen (than already exists).


I think that the repr was the only bit that could be shared between
IntervalArray & the Index base class. Could you point out another instance
of duplicate code (in that PR)? I think all the others you pointed out
(e.g. take) are interval-specific.

On Tue, Apr 10, 2018 at 7:16 AM, Jeff Reback <notifications@github.com>
wrote:

> Given discussion in #19696
> <https://github.com/pandas-dev/pandas/issues/19696> and #20611
> <https://github.com/pandas-dev/pandas/pull/20611>, I think we will rather
> go with composition than subclassing. In that case, I am not sure I am in
> favor of the mixins you are adding in this PR.
>
> I think you ought to re-read my comments.
>
> Index *must* follow the EA protocol otherwise we have tons of duplicated
> code. This does NOT preclude have a composed internal implementation of an
> EA Array itself. In fact this is a very nice way to do things.
>
> This is my point of using mixins. One of the primary goals is to simplify
> the internals of pandas itself by using a standardized protocol. My
> continued pushback on adding new code is for exactly this reason. The
> implementors of the current internal EA's (categorical, proposed
> intervals), need to refactor internal code. Otherwise *even* more
> technical debt is going to happen (than already exists).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/19902#issuecomment-380077419>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHInLEAJW3WDUTzarX-p952uAtrwYYks5tnKKSgaJpZM4SSsiD>
> .
>

Typing with thumbs today.

@TomAugspurger I cant speak to the IntervalArray topic, but all of the Index arithmetic methods belong in EA subclasses.  ATM numeric index and series/frame arithmetic code is duplicated (and still not entirely internally consistent)
> all of the Index arithmetic methods belong in EA subclasses.

@jbrockmendel But does that necessarily mean that they should be in a mixin? The Index class can also get access to those operations without calling the exact same code? Eg we could have `TimedeltaIndex.__add__` delegate to `TimedeltaArray.__add__` and wrap the result? Then all arithmetic methods can still be in the array class, without having the need that those methods work for both Index as Array. 

(for interval, we didn't have such a discussion there yet, as the WIP of Tom does not have any aritmethic operations yet, so that it was not needed for Interval is indeed not necessarily a good reason)
> Eg we could have `TimedeltaIndex.__add__` delegate to `TimedeltaArray.__add__` and wrap the result? 

That's the plan regardless of which route (inheritance vs composition) is taken.  This can either end up looking something like:

```
def __add__(self, other):
    result = TimedeltaArray.__add__(self, other)
    res_name = ops.get_result_name(self, other)
    return Index(result, name=res_name)
```

or

```
def __add__(self, other):
    result = self._eavalues + other
    res_name = ops.get_result_name(self, other)
    return Index(result, name=res_name)
```

Which way the dispatch works is essentially orthogonal to the Array implementation.

<b>Update</b> I have a preference for the inheritance version largely because `_?[a-z]*_?values` attributes are overloaded as it is.
That looks good, but then my question is: why does the `Index` class (eg `TimedeltaIndex` for the example above) need to have the methods like `_add_datelike`, `_add_offset`, `_add_delta`, `_add_delta_td`, ... which are in this PR in the mixin ? 
As those methods are only needed to implement `TimedeltaArray.__add__`, and `TimedeltaIndex.__add__` will use `TimedeltaArray.__add__` directly and not those specific methods?
@jorisvandenbossche Under inheritance `TimedeltaIndex` would not need to use those methods directly (and with composition would not have those methods at all).  Simplifying the `TimedeltaIndex` namespace is an argument in favor of composition.

Don't read too much into the fact that in this PR `DatetimelikeIndexMixin` inherits from `DatetimelikeArrayMixin`.  That's just done to keep `DatetimelikeIndexMixin` internally coherent.
But is it then the end-goal that `DatetimeIndexOpsMixin` (or `DatetimeIndex`) does not inherit from `DatetimelikeArrayMixin` ? 
Whether this would already be achieved before merging this PR, or done afterwards when the actual dispatching of arithmetic methods to the array is done, is another discussion, but just want to get the understanding correct.
The methods moved to `DatetimelikeArrayMixin` are all _necessary_ for the arithmetic/comparison operations, but are not necessary _exclusively_ for those operations (e.g. `__iter__`).  If we go down the composition path, we would break `DatetimelikeArrayMixin` up into two pieces and only mix one of them into `DatetimelikeIndexMixin`.


But even `TimedeltaIndex.__iter__` could also do `return iter(self._values)` instead of sharing the actual implementation with `TimedeltaArray`, in which case it can use the base Index method, instead of needing to override it in TimedeltaIndex. And the same holds for `_isnan` and `hasnans`. From the current mixin, I don't think there will be much left?

Based on a quick perusal it looks like `freqstr`, `inferred_freq`, `_maybe_mask_results`, (`_box_func` if we're sticklers) are needed by the Index classes.

The `__iter__` example works if we go with composition and `TimedeltaIndex._values` returns a `TimedeltaArray`, yah.

Given that a) the decision about inheritance vs composition appears un-settled and b) this PR is agnostic about it anyway, I don't see why this shouldn't go forward.
>  a) the decision about inheritance vs composition appears un-settled

In https://github.com/pandas-dev/pandas/pull/20611 we are going with composition, and in https://github.com/pandas-dev/pandas/issues/19696 the discussion also seems to go to composition, so best to respond on those issues if you don't agree with that.

> Update I have a preference for the inheritance version largely because `_?[a-z]*_?values` attributes are overloaded as it is.

Can you explain this further? 
If we are using composition, `<type>Index._values` (or `_data`, just need to choose a name and stick with it) == `<type>Array`, and `<type>Index._ndarray_values` could be delegated to `<type>Array._ndarray_values` or `<type>Array.__array__`. I think that can be very clear. I don't really see what would be better with inheritance on this specific point.

> this PR is agnostic about it anyway, I don't see why this shouldn't go forward.

For me the reasons to not merge it in the *current* state is because: it adds a mixin we in the end might want to remove, it adds a lot of methods to the array classes we don't want there IMO (see the inline comments above), and it let's the Index classes subclass from the Array classes which we also might not want to do in the end.


Supposing this gets killed off, do you have any thoughts for how to go from here to having the relevant Array subclasses implemented?

> In #20611 we are going with composition, and in #19696 the discussion also seems to go to composition

Good to know, thanks for passing that on.  My preference for inheritance is pretty minor, so I'm a happy camper.

> Can you explain this further?

The values/_values/data/_data/... issue is probably simpler than I imagine it, but I often find myself having to go double-check what these attributes refer to, since it can be slightly different depending on if I'm looking at an Index or `Series` or `Block` or `Series[datetime64tz]`.  This is largely tangential, and you're right that settling on a naming convention should resolve it.

> For me the reasons to not merge it in the current state is because: it adds a mixin we in the end might want to remove, it adds a lot of methods to the array classes we don't want there IMO

For the mixin concern, that's fair.  I see it as a price-of-doing-business given that the goal of this PR is very specifically to be cut/paste.

For the unwanted methods, all the marked methods are needed for the arithmetic ops.  They can be renamed later if needbe (and then restored on `DatetimelikeIndexMixin`).

> and it let's the Index classes subclass from the Array classes which we also might not want to do in the end.

I don't understand why that is a negative in and of itself.