Datetimelike Array Refactor

[TomAugspurger]

A master issue, to help keep track of things.

High-level, I think we have two things to sort out

1. Design our datetimelike Arrays (DatetimeArray, TimedeltaArray, PeriodArray)
2. A plan for getting from master to an implementation of that design.

1. Design

We have a few things to sort out

a. Composition vs. inheritance of Index / Series and array classes
b. ...

2. Implementation Plan

A few options

a. Big PR implementing one or more arrays, followed by smaller cleanup PRs
b. Incremental PRs (implementing most of the interface on the *ArrayMixin classes), followed by a final PR making the switch
c. ...

Project board with the relevant discussions / PRs: https://github.com/pandas-dev/pandas/projects/4

[TomAugspurger]

cc @jreback @jorisvandenbossche @jbrockmendel

OK I wanted to open this issue because I've had trouble keeping up with all the
goings-on around this (and I was getting tired of rebasing the WIP PeriodArray
PR). So I'm hoping we can come to a rough consensus on these outstanding
discussion, and a concrete plan of how to get from master to there.

If necessary, we can move to a google doc or something to collaboratively edit
the design doc. I don't have a preference. Feel free to add discussion points
to the "outstanding discussions" list.

I'll try to write up my current thoughts later today.

[jbrockmendel]

You're right (and Joris has expressed this elsewhere) that this conversation has splintered across a lot of places and centralization will help.

I'm a big fan of breaking up big problems into smaller more manageable problems (as evidenced by #23159, small-step tslibs refactor, doomed improvement efforts in statsmodels, ...). Are there any logically independent parts of the problem that can be split off?

FWIW:
#23173 I opened specifically to split discussion-requiring pieces off from the other PRs
#22535 does touch the mentioned files, but should be orthogonal to the topics under discussion.
#23113 I hope we can push through relatively easily, since it is needed for DatetimeArray + DateOffset addition to work.

[jorisvandenbossche]

I mentioned it on one of the PRs, but is it OK for you guys to agree on not merging any of the open PRs, before we have some agreement on the way forward?

@jbrockmendel I understand you want to keep working on those PRs you opened (and it's also great that you do so much for pandas!), but let's maybe take a short pause doing new PRs until we agree on how we want to get to the finish-line here (in that sense: can you answer my mail regarding thursday?)
Of course, doing some PRs might help to explore certain options, show how things can be done, etc. So that is certainly fine, just not finalizing / merging.

[jbrockmendel]

Sounds good.

[TomAugspurger]

Composition vs. inheritance.

The case for composition:

1. I see EAs as being at the same level as ndararys. Series & Index box an array
   (ndarray or EA). I think it'd be strange for some indexes be instances
   of an EA, depending on the dtype.
2. I think that the calls have only been for specific Index classes to inherit
   from EA. We'll limit the rest of this discussion to just Index, but I think
   that fact is an argument for composition.
3. Indexes have names, arrays don't. That means we'll have similar, but
   different, function signatures and calls to pass names where they need to go.
4. Some methods common to both Index and EA have different semantics (e.g.
   shift).

The case against:

1. Additional boilerplate code for dispatching an operation from the Series
   or Index to the EA.
2. ... (make your case here)

---

Since I'm pro-composition, I'll point out that I think we're required to have
that dispatching code anyway. If we're a Series / Index backed by an ndarray,
then we dispatch to numpy. If we're backed by an EA, then we dispatch to it. Or,
to put it another way, Index / Series ops simply dispatch to the ops of the
underlying array.

[jbrockmendel]

If there is a nice solution to the immutability/caching issue, then I can get on board with just-composition. Until then, I think both is the way to go for now. i.e. PeriodIndex subclasses PeriodArray and PeriodIndex.values returns a PeriodArray.

Your point 1) is the one I find most compelling. It would be really nice if .values had a consistent definition as always-lossless.

[TomAugspurger]

I'm not sure how doing both would work in practice. I don't have a good sense for what complications that's likely to create.

But, I think that won't be necessary. I think we can manage to cache attributes like hasnans or _isnan on the Array, and invalidate that cache when necessary (__setitem__).

It would be really nice if .values had a consistent definition as always-lossless.

Agreed. At this point, I think that's our only hope of having a consistent definition for what .values is.

[TomAugspurger]

On the caching point, this seems to work

diff --git a/pandas/core/arrays/period.py b/pandas/core/arrays/period.py
index 24d4b6e55..23a5845f0 100644
--- a/pandas/core/arrays/period.py
+++ b/pandas/core/arrays/period.py
@@ -465,6 +465,14 @@ class PeriodArray(dtl.DatetimeLikeArrayMixin, ExtensionArray):
                    "Got '{}' instead.".format(type(value).__name__))
             raise TypeError(msg)
         self._data[key] = value
+        self._invalidate_cache()
+
+    def _invalidate_cache(self):
+        self._cache = {}
+
+    @cache_readonly
+    def hasnans(self):
+        return self.isna().any()
 
     def take(self, indices, allow_fill=False, fill_value=None):
         from pandas.core.algorithms import take

There may be edge cases, or cases were we could skip invalidating the cache, but it's at least feasible.

It didn't come across in the diff, but the call to _invalidate_cache comes from PeriodArray.__setitem__.

"tests":

without the invalidation

In [1]: import pandas as pd
ar
In [2]: arr = pd.core.arrays.period_array(['2000', None], 'D')

In [3]: arr.hasnans
Out[3]: True

In [4]: arr[1] = pd.Period(2000, 'D')

In [5]: arr.hasnans
Out[5]: True

with invalidation

In [1]: import pandas as pd

In [2]: arr = pd.core.arrays.period_array(['2000', None], 'D')

In [3]: arr.hasnans
Out[3]: True

In [4]: arr[1] = pd.Period(2000, 'D')

In [5]: arr.hasnans
Out[5]: False

[jorisvandenbossche]

It overlaps with the first point of Tom, but an additional case for composition / disadvantage of inheritance:

• A clear data model that you can explain and reason about (Series and Index are containers of array-likes), while the current inheritance is overly complicated and very opaque IMO.

I personally also don't see any compelling case for inheritance (also the dispatching is not a reason IMO, as we need to it anyway for Series)

So personally, unless someone now actually makes an extensive and detailed argument for inheritance, I would propose to leave this discussion behind us and focus on how to solve possible remaining issues with the composition structure (the constructors, the caching).

[jorisvandenbossche]

For the inheritance vs composition, we actually have examples to look at in practice: interval and categorical already do composition, while the datetimelikes are currently a kind of inheritance. I think the current datetimelike implementation shows the complexity of this.

[jorisvandenbossche]

Related to the caching issue, I think there are several options:

1. Assess if it is actually significant (eg to decide if it is a blocker on the PeriodArray PR). From a quick timing, calculating the _isnan property the first time takes about 5% of the time to do DatetimeIndex() + Timedelta() arithmetic operation (whether it has a similar impact when we actually would no longer cache is not necessarily the case of course, should be checked further)

2. Implement caching on the Array objects. I think this is certainly feasible, as Tom also explores above.

3. Factor the operations that make use of caching out into functions instead of methods (like the current _add_delta methods), that is shared between Index and Array, and where eg the (cached or not) mask can be passed as an argument.

So I think there is certainly a solution possible, and my first point about "is this important" is then more to know if this is essential to already have in an initial "big split" PR like the PeriodArray PR, or if this can be left for a follow-up PR.

[jorisvandenbossche]

Warning, long post coming.

My proposal for a possible way forward, let's call it the "minimally big" PR with follow-up PRs proposal:

• For each Array class, we do a "minimally big-split" PR:
  • a PR that does the actual split of the current inheritance structure into separate Index and Array objects
  • but as minimal as possible:
    • as a minimum get the tests passing
    • and with agreement on the bigger design questions (different class structures, the constructors, some minimal design questions we can identify we want to agree on at this stage)
  • I think this should try to mainly be "moving around code" (and adding some needed dispatching), to the extent possible of course to satisfy the above points, but meaning here that the goal of those PRs should not be to clean-up implementations, or to focus on deduplication, smart decorators, etc
  • We do this for each Array class, and I think we should try to keep those as independent as possible for each Array class (Period, Datetime, maybe Timedelta?). This will of course mean some duplication between each Array, but at this stage this is OK. Doing this will keep the picture as clear as possible in each PR, and this can be cleaned-up later on.
  • I think the current PeriodArray PR of Tom is a good start for such a PR.
• During reviewing those PRs, we identify and track follow-up issues we agree on we want, but are OK to keep for a next stage. Things like: consolidating common patterns that can be shared between the EA based Index classes, or more specifically patterns that can be shared between the datetimelike Array classes; clean-up of the Index constructors (the simple_new, shallow_copy(_with_infer) story); restructuring of the tests (how to share tests between Index/Array?); other clean-up ideas, etc
• After those "minimal big splits" are merged, we can start doing follow-up PRs based on the ideas that have formed during reviewing the previous PRs. This can now be smaller, more independent and easier to review PRs.
  Those follow-up PRs can be easier to review, having clear goals identified before, and fitting already in the final structure giving better context to review the PR.

My reasoning to go for the above way forward compared to a "first smaller clean-up PRs, then split", are the following:

• The current smaller PRs are very difficult to judge and review because the big picture (the final code structure) is still missing
• Even after doing PRs preparing the split, the actual split PR will still be big
• Many of the PRs that are now open which Tom listed in the initial issue, are things like deduplicating, moving common code to helper functions, ... All good things! But those can also perfectly be done after the initial split PR.

This proposal would mean that master will be in a temporary "messy" state (but still green of course). If we find this a problem, we can always first merge those PRs to a refactor branch, and only after some of the follow-up PRs have been done to that branch as well, merge it into master.
(personally, I don't think that is needed as long as we keep master green, as it will only give additional complexity to keep master and that branch in sync).

Doing the above, would in practice mean: first focus on some of the design discussions (eg the design of the constructors, and other issues mentioned in the top post), focus on doing and reviewing the actual splits, and for now wait with the other smaller PRs.
And also in this proposal I think there is enough work to share for multiple people working at the same time (without trying to get things out of the big PRs to independent PRs). Doing those discussions, reviewing PRs, doing the Datetime or Timedelta Array, etc are also all things that need to be done and take time.

---

So please give your thoughts about this proposal, feedback, alternative proposals, ...

[jbrockmendel]

If patching __setitem__ works robustly I think that would be a great solution. It would probably be useful elsewhere, too. The fact that it would be so useful makes me think it must be harder than it looks, otherwise it would have been done long ago. But I'd be happy to be proven wrong.

A clear data model that you can explain and reason about

If we were to go whole-hog on inheritance, the data model would be "An Index is an Array with some additional fancy indexing/join/set methods (and a Block is an Array with mgr_locs attribute)". That said, conditional on the __setitem__ thing working out, I'm happy to end this part of the conversation.

Factor the operations that make use of caching out into functions instead of methods

That's an interesting idea. I'd like to give it some thought before forming an opinion.

The current smaller PRs are very difficult to judge and review because the big picture (the final code structure) is still missing

Are they though? DatetimeArray does need to_period and to_perioddelta for arithmetic operations to work. Is there any scenario in which we dont want all three arrays to have take implemented? If anything, isolating them makes it clear what depends on what, avoids the phenomenon in big PRs where I have to ask "is this related to everything else?"

---

My main objection to the Big PR is that it precludes working in parallel. If there was something about the implementation that required it be done All At Once that would be another matter, but as it is there is a lot of non-difficult stuff we can get out of the way before making final decisions about caching and constructors. i.e. the "Minimal Big Split" can be made more minimal.

Suppose hypothetically that two things both turn out to be more difficult than expected: the __setitem__ patching and the arithmetic methods. In the Big/Minimally Big model, everything is on hold until a single PR gets all of it right. In a parallel model, I can figure out the arithmetic while Tom figures out the caching. (For the sake of the example I'm assuming that the ways in which they are unexpectedly difficult are logically independent)

---

The status quo is that non-#22862 PRs are on hold. While not my first choice, I'd rather see that move forward than go in circles here. Let's see what jreback has to say and reconnoiter.