REF/DEPR: DatetimeIndex constructor
State: closed

This is preliminary to implementing DatetimeArray._from_sequence.  The attempt to implement that directly (i.e. without existing circularity) failed, so trying it as a 2-parter.

Similar to #23539, this deprecates the option of passing timedelta64 data to DatetimeIndex.

There are a few places where I've made TODO notes of the "is this really intentional?" form; I'll point this out in inline comments.

@mroeschke I edited a couple of docstrings for the recently-implemented "nonexistent" kwarg, can you confirm that I didn't mess them up?

Labels:
name: Datetime
description: Datetime data dtype
name: Deprecate
description: Functionality to remove in pandas

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/f8efbeffb63d119b69d90673573f274afe88905c/pandas/core/arrays/datetimes.py) !

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

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

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


azure failure is a Hypothesis failure
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23675?src=pr&el=h1) Report
> Merging [#23675](https://codecov.io/gh/pandas-dev/pandas/pull/23675?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/022f458ee858721d366c27dd88940d69383ab960?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `94.73%`.

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

```diff
@@            Coverage Diff             @@
##           master   #23675      +/-   ##
==========================================
+ Coverage   92.31%   92.31%   +<.01%     
==========================================
  Files         161      161              
  Lines       51562    51586      +24     
==========================================
+ Hits        47599    47622      +23     
- Misses       3963     3964       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.71% <94.73%> (ø)` | :arrow_up: |
| #single | `42.44% <63.15%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23675?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23675/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.55% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/tools/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23675/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvZGF0ZXRpbWVzLnB5) | `85.27% <92.3%> (-0.27%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23675/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `98.43% <94.73%> (+0.06%)` | :arrow_up: |

------

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

Merged master to fix the merge conflict.
Travis error looks unrelated
@jbrockmendel Can you give a bit more context here? Eg, why are you adding so much new code (while there are actually no user facing changes, I suppose, except from the deprecation) ? 
What is the rationale of not using ``to_datetime`` (if we refactor things, I would rather think it is to share *more* implemenation) ?
> Can you give a bit more context here? Eg, why are you adding so much new code (while there are actually no user facing changes, I suppose, except from the deprecation) ?
What is the rationale of not using to_datetime (if we refactor things, I would rather think it is to share more implemenation) ?

Sharing implementation is fine, circularity is not.  The fact that `to_datetime` calls the `DatetimeIndex` constructor and vice-versa makes it very difficult to sort out (in fact I spent a big chunk of Monday trying to untangle it in one pass and couldn't) what happens in which case.  The addition of new code is to make these behaviors more clear/explicit.

Moreover I am not convinced that the float_dtype behavior is intentional.

There is an issue (need to dig it up) reporting a bug in passing a Categorical to `DatetimeIndex`.  Point being: there is value in being more explicit about what we are doing and why.
> Sharing implementation is fine, circularity is not.

Yes, but that circularity already existed. What changes did you try to make that it became more problematic?

>  The addition of new code is to make these behaviors more clear/explicit.

But it also means duplicating in `DatetimeIndex` things that are already handled in `to_datetime`. Not sharing it will much more easily lead to a future change leading to inconsistencies between both?

For the TimedeltaIndex PR, you made some common functions that were used by both. Is a similar pattern not possible here?

> For the TimedeltaIndex PR, you made some common functions that were used by both. Is a similar pattern not possible here?

That's the goal behind #23702.
> That's the goal behind #23702.

So the idea is to merge that PR first and then update this one?
> So the idea is to merge that PR first and then update this one?

Probably.  The idea changes a few times a day.
OK, fun new changes...

- Deprecation warning for timedelta64 data is now in both `to_datetime` and `DatetimeIndex.__new__`
- ditto raising on PeriodDtype (still need tests)
- Usage of `to_datetime` in `DatetimeIndex.__new__` is now entirely removed.  Instead it calls objects_to_datetime64ns which contains the relevant part of to_datetime
- We _could_ make `to_datetime` call this too, but I'm hesitant to do so because a) it would be really ugly and b) I'm still sorting out the ways in which `to_datetime` and `DatetimeIndex.__new__` behave differently, which is a PITA.  Fixing this is going to involve both API changes and major cleanup in `array_to_datetime`, both of which are outside the scope of this PR.
The follow up to this will get rid of a kludge in the DTA constructor
Rebased following #23916
Travis fail is unrelated server timeout
Looks like #23937 flubbed a docstring in arrays.datetimes.  Fixed that after rebasing here.
Azure fail looks unrelated
> yeah i agree this [treating floats as wall-times, ints as UTC] seems a little odd, these should just be cast to integer i think.

Change behavior here, deprecation cycle, or follow-up?
gentle ping.  Getting the constructors done will noticeably tighten the scope of #24024.
> actually DTI should just return a Index in that case, otherwise this is very confusing. I believe you have an issue talking about mixed-timestatmps. These should always just return an Index of objects. No conversion / inference should happen at all.

I'm -0.5 on having DatetimeIndex.__new__ return an object Index, would rather have it raise.  Either way, that is a design change that should be orthogonal to this PR.
> I'm -0.5 on having DatetimeIndex.new return an object Index, would rather have it raise. Either way, that is a design change that should be orthogonal to this PR.

sure (and raising is prob correct)
This is a blocker for DatetimeArray._from_sequence, so probably merits a 0.24.0 milestone tag
@jbrockmendel ok thanks. I realy really want to limit scope to things that we absolutely need to pushout the Datetime and associated EA's. sure there are other things to do, but theses need much more consideration that we have time for. (basically I am saying, this code IMHO is not strictly necessary for this refactor, though its a nice to do).