REF/ENH: Constructors for DatetimeArray/TimedeltaArray
State: closed

Big push on the constructors for DatetimeArray/TimedeltaArray, some progress de-duplicating code from their Index counterparts.

As discussed elsewhere, adds `dtype` to `PeriodArray.__init__`

@TomAugspurger can you confirm that the `_from_sequence`s implemented here handle the right cases?  It wasn't obvious if object-dtyped array/indexes were supposed to be handled there, but combining those cases was too clean to overlook.

Small fix in DatetimeArray comparison methods, just enough to make the tests work.  A separate PR forthcoming PR will do a more thorough fix/improvements of those.

One non-obvious point on which input would be especially welcome is how/when to use the `copy` kwarg in such a way as to copy at-most-once.  (related: deep_copy_if_needed and #21907)

#23491 found during this process, will be addressed in a follow-up.

Labels:
name: Datetime
description: Datetime data dtype
name: Timedelta
description: Timedelta data type

Comments:
Hello @jbrockmendel! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimelike.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/core/arrays/datetimelike.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/core/arrays/datetimes.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/period.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/core/arrays/period.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/timedeltas.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/core/arrays/timedeltas.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/core/indexes/datetimes.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/timedeltas.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/core/indexes/timedeltas.py) !

 - There are no PEP8 issues in the file [`pandas/tests/arrays/test_datetimes.py`](https://github.com/jbrockmendel/pandas/blob/1a015f6803887bf88d9afadecc228daa7ae3fe9c/pandas/tests/arrays/test_datetimes.py) !



##### Comment last updated on November 04, 2018 at 18:58 Hours UTC
Good comments, thanks.  Also some linting mistakes I need to fix.  I'll make another pass and comment when this is ready to be looked at.
Updated with (most) requested edits, basic tests for TimedeltaArray (and fixes to make them pass, particularly implementation of is_monotonic_increasing etc)
Pushed commits fixing tests, also implemented `maybe_validate_freq`.  I think this can be handled more cleverly in some cases, will look into this.  There was also an Issue about removing the `verify_integrity` argument from some of the constructors.  Now would be decent time to do so.
> @TomAugspurger can you confirm that the _from_sequences implemented here handle the right cases? It wasn't obvious if object-dtyped array/indexes were supposed to be handled there, but combining those cases was too clean to overlook.

It should handle any sequence where the scalar types are instances of `ExtensionArray.dtype.type` or NA. So yes, object-dtype arrays should be handled.
@jorisvandenbossche Your attention has been specifically requested in #23514, whereas I am finding it increasingly frustrating.  I propose we step back from this conversation so I can spend some time addressing the subset of comments on which there is consensus.
Maybe we can spend a bit of time building consensus on a direction forward? I'll try to build my own thoughts here on a proposal, as a response / concurrence to 
https://github.com/pandas-dev/pandas/issues/23185#issuecomment-435592816.

FWIW, everything on the TimedeltaArray / DatetimeArray is on my critical path, so I'm going to prioritize reviewing your PRs over everything else.
> [Tom] I thought we were all on board with the goal of the DatetimelikeArray.init being no inference and no copy.

@jbrockmendel you have to see my comments in the same light. I was assuming we had this discussion and agreed on the design of the constructors, so I was wondering why we couldn't already follow that decision in this PR instead of deferring to a follow-up. 
So this mis-assumption might have made things more frustrating than needed for *both* of us.

> Your attention has been specifically requested in #23514, whereas I am finding it increasingly frustrating.

Just as for Tom, this PR (and all issues related to the EA refactor) is high on my priority list, so I will still put time on this topic (which is more than writing comments here). 
So you can expect a lot more comments to come (not necessarily now, but when you had the time to update the PR). But please don't see this as a bad sign, but rather as an opportunity to move forward quickly (and as an indicator of the importance of this refactor). Tom's PRs on the SparseArray and PeriodArray also saw a huge amount of comments and discussion, but I think that significantly improved the PRs and moved us relatively quickly to a shared understanding. 
And to be clear, it is no problem that it takes some time to process those comments.

If you have specific feedback on how I can make my comments less frustrating apart from the above, I am honestly all ears.

---

But indeed, let's first try to build consensus on the fundamental design questions. I would propose to not do that here on this PR but on the general issue https://github.com/pandas-dev/pandas/issues/23185 and the constructor split-off issue https://github.com/pandas-dev/pandas/issues/23212, so we can keep the discussions in this PR on technical implementation details of things we in general already agree on.

I would personally propose to have a chat about this, as a more effective way to discuss things. But @jbrockmendel, I think that only makes sense if you can fix the audio issues we were having last time. If that is not possible right now, I would propose to do it on chat at a given time to at least have it a bit more interactive than on github.
> If you have specific feedback on how I can make my comments less frustrating apart from the above, I am honestly all ears.

I appreciate the consideration.  At this point I think the short-term solution is for me to take a half-step back and disengage.  The request that you do the same was primarily so you don't feel that I'm becoming non-responsive.

Medium-run, I very much think the priority should be getting enough of this in place that we can get the tests working.  Later if you want to implement `datetime_array` mirroring `period_array` or rename `__init__` to `_from_sequence` (I'll be renaming `_from_sequence` to `_from_objects` or something) then that will be _entirely trivial_ to do.

I'll be going AFK (at least on this thread) for a few hours.
Good news, for a certain value of "good".   In the next pass of de-duplication I found some subtle bugs in the TimedeltaIndex construction, including some that we are specifically testing.  So there will be at least one fixing-things PR before this comes back to the forefront.  

In the interim, we can make incremental progress on orthogonal DTA/TDA code in #23415 and #23502.
> So there will be at least one fixing-things PR before this comes back to the forefront.

I know I am not at all in the position to force things, so I will just give my opinion (and take it as that :-)): I think we should rather focus our effort on moving forward this PR (eg start trying to reach consensus on the discussion points above). 
It's really good you found those bugs, and we should certainly fix them, but those are not *critical* for the refactor, while the things discussed here are.

> In the interim, we can make incremental progress on orthogonal DTA/TDA code in #23415 and #23502.

I thought you mentioned #23415 was kind of dormant for now? On #23502 I added some additional comments.
> I thought you mentioned #23415 was kind of dormant for now?

You thought correctly, but it may be worthwhile to un-dormant-ize it so as to maintain forward momentum while bugs get sorted out.

> but those are not _critical_ for the refactor, while the things discussed here are.

Disagree on both counts.  Without the bugs being fixed, we can't institute meaningful tests for the array classes.  On the other hand, whether we rename `__init__/_simple_new` to `_from_sequence/__init__` can absolutely be postponed until after we have a fully-working implementation.
Closing.  I’ll salvage any useful tests in the PR that comes after #23539.