Implement DatetimeArray._from_sequence
State: closed

Removes dependence of `DatetimeArray.__new__` on `DatetimeIndex`.  De-duplicated `DatetimeIndex.__new__`/`DatetimeArray.__new__`.

The contents of `DatetimeArray._from_sequence` are basically just moved from `DatetimeIndex.__new__`.  This is feasible because #23675 disentangled `to_datetime` from `DatetimeIndex.__new__`.

cc @TomAugspurger this is the last thing on my todo list for DTA/TDA.  LMK if I can be helpful with the composition transition.

Labels:
name: Datetime
description: Datetime data dtype
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

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

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

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


> LMK if I can be helpful with the composition transition.

Mind if I ping you on specific xfails that I've added in that PR after this, 
https://github.com/pandas-dev/pandas/pull/23601, and 
https://github.com/pandas-dev/pandas/pull/23990 are merged? I'm starting to whittle them down.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=h1) Report
> Merging [#24074](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/08395af43b813b69b1cf038d260104cfeeb3e787?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `79.66%`.

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

```diff
@@            Coverage Diff             @@
##           master   #24074      +/-   ##
==========================================
- Coverage   42.38%   42.38%   -0.01%     
==========================================
  Files         161      161              
  Lines       51701    51691      -10     
==========================================
- Hits        21914    21907       -7     
+ Misses      29787    29784       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `42.38% <79.66%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `49.6% <33.33%> (-2.73%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `65.56% <88%> (+1.76%)` | :arrow_up: |
| [pandas/tseries/frequencies.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvZnJlcXVlbmNpZXMucHk=) | `70.8% <0%> (+0.72%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24074?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/24074?src=pr&el=footer). Last update [08395af...be745aa](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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

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

```diff
@@            Coverage Diff             @@
##           master   #24074      +/-   ##
==========================================
- Coverage   42.38%   42.38%   -0.01%     
==========================================
  Files         161      161              
  Lines       51701    51691      -10     
==========================================
- Hits        21914    21907       -7     
+ Misses      29787    29784       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `42.38% <79.66%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `49.6% <33.33%> (-2.73%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `65.56% <88%> (+1.76%)` | :arrow_up: |
| [pandas/tseries/frequencies.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvZnJlcXVlbmNpZXMucHk=) | `70.8% <0%> (+0.72%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24074?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/24074?src=pr&el=footer). Last update [08395af...be745aa](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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

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

```diff
@@            Coverage Diff             @@
##           master   #24074      +/-   ##
==========================================
- Coverage   42.38%   42.38%   -0.01%     
==========================================
  Files         161      161              
  Lines       51701    51691      -10     
==========================================
- Hits        21914    21907       -7     
+ Misses      29787    29784       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `42.38% <79.66%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `49.6% <33.33%> (-2.73%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `65.56% <88%> (+1.76%)` | :arrow_up: |
| [pandas/tseries/frequencies.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvZnJlcXVlbmNpZXMucHk=) | `70.8% <0%> (+0.72%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24074?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/24074?src=pr&el=footer). Last update [08395af...be745aa](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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

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

```diff
@@            Coverage Diff             @@
##           master   #24074      +/-   ##
==========================================
+ Coverage    92.2%    92.2%   +<.01%     
==========================================
  Files         162      162              
  Lines       51729    51717      -12     
==========================================
- Hits        47697    47686      -11     
+ Misses       4032     4031       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.6% <99.18%> (-0.01%)` | :arrow_down: |
| #single | `43.04% <77.04%> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `96.41% <100%> (+0.06%)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.06% <100%> (-0.02%)` | :arrow_down: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `87.13% <100%> (-0.15%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.32% <100%> (-0.24%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `98.56% <100%> (+0.31%)` | :arrow_up: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `98.29% <92.3%> (-0.18%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.27% <0%> (-0.06%)` | :arrow_down: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/24074/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.26% <0%> (+0.13%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24074?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/24074?src=pr&el=footer). Last update [8ea7744...9d7cb39](https://codecov.io/gh/pandas-dev/pandas/pull/24074?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> Mind if I ping you on specific xfails that I've added 

Sounds good.
On the function signature, it would be *nice* if it matched the interface, but that's not strictly necessary. Apparently, https://en.wikipedia.org/wiki/Liskov_substitution_principle is the "rule" here, and I don't think adding optional arguments like `freq=None`, which are mere optimizations, breaks that rule. Things like `dayfirst` probably do violate it though :/

Fundamentally, I think this is because we're overloading `_from_sequence` to handled things that `ExtensionArray._from_sequence` doesn't expect / require. According to the interface, the type of `scalars` would be, roughly, `Sequence[Timestamp]`. We're also allowing things like `Sequence[strings-that-can-maybe-be-parsed-as-timestamps]`.

With PeriodArray, we got around this with a top-level `period_array` method that handles all the mess that people can throw at us, reserving `_from_sequence` for `Sequence[Period]`, and `__init__` for simply setting the attributes.

What do you think about moving this PR's current `_from_sequence` to a different name (not sure what to call it, maybe a top-level `datetime_array` to mirror `period_array`?

I see that `TimedeltaArray._from_sequnce` also has some extra args (`freq`, `unit`, and the positional arguments name is different).
> and the positional arguments name is different

definitely +1 on having these match

> What do you think about moving this PR's current _from_sequence to a different name (not sure what to call it, maybe a top-level datetime_array to mirror period_array?

I'm on board with separating _most_ of the method out into a `sequence_to_dt64ns` mirroring the `sequence_to_td64ns` we have in timedeltas.

I still think having `period_array` instead of having the `PeriodArray` constructor be user-friendly is silly.  I've made my peace with not getting my way on this one, but draw the line at implementing it myself.  It will be easy for e.g. Joris to "fix" these in a follow-up.

(plus there's also `to_datetime`.  -1 on proliferation when `DatetimeArray.__init__` is an obvious fit and `DatetimeArray._from_sequence` also exists)

`sequence_to_dt64ns` notwithstanding, I think the main outstanding concern for this PR is the pickle/verify_integrity thing, which should be handled by either a) @jreback being OK with the change to `_new_DatetimeIndex` implemented here or b) #24096 getting sorted out.
That's all totally fair.

> I think the main outstanding concern for this PR is the pickle/verify_integrity thing, which should be handled by either a) @jreback being OK with the change to _new_DatetimeIndex implemented here or b) #24096 getting sorted out

Of those two, I'm not sure which is preferred. Some scattered observations

1. if the only thing that failed on #24096 was the recent test from https://github.com/pandas-dev/pandas/pull/23956, then feel free to ignore / break that. I suspect that the groupby issue segfaulting there isn't properly fixed yet, but...
2. I think that *all* of these pickle concerns are going to go away once we just use `__init__` instead of `__new__`. It may be worth trying to see if we can do that before #24024.
I'm about to push a new commit, the relevant changes being:
- updated `TimedeltaArray._from_sequence` signature to have positional arguments match `ExtensionArray._from_sequence`
- separated most of `DatetimeArray._from_sequence` into `sequence_to_dt64ns`, mirroring timedelta version `sequence_to_td64ns`
- moved `validate_tz_from_dtype` from `arrays.datetimelike` to `arrays.datetimes`, since it is only used there
- added to `validate_tz_from_dtype` a check for tz-naive dtype and non-None tz, which lets us move that particular check to before the call to `_simple_new`
- implemented `datetimelike.validate_inferred_freq` to share some more validation code between `TimedeltaArray._from_sequence` and `DatetimeArray._from_sequence`

`sequence_to_dt64ns` still needs a docstring
> if the only thing that failed on #24096 was the recent test from #23956,

That test failed here, not in #24096.  It was fixed by avoiding calling `data = data.view(_NS_DTYPE)` in the case where `data.dtype` is already `M8[ns]`.  The difference in the code amounts to one indentation.  Not worth worrying about.

> I think that all of these pickle concerns are going to go away once we just use __init__ instead of __new__. It may be worth trying to see if we can do that before #24024.

I don't see how that will work, but pickling is an area where I frequently find new and exciting ways to screw up.  My preference would be to merge this with the small `_new_DatetimeIndex` edits and revisit it after #24024.  This would make #24096 not-a-blocker and let me go through those constructors a little more thoroughly (motivated #24100)
ok, rebase just in case here
Rebased.  Haven't reverted the _new_DatetimeIndex change because there is one other case that still fails: there is an overflow in _generate_range that we're not handling correctly.  I'll address that in a separate PR before long.
thanks @jbrockmendel merging to unblock things. but as they say in boxing: keep it clean! hahah