EA: revert treatment of i8values
State: closed

One option for #24559, with some spillover into #24567.

Restores freq validation (needs test)
Retains a _private_ fastpath

- [x] Closes #24615


Labels:
name: Datetime
description: Datetime data dtype
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
No idea how we would go about fixing the failing feather tests.
@TomAugspurger any idea about the feather thing.  AFAICT any change that restores the DTA/Timestamp i8 equivalence will run into this problem.
> AFAICT any change that restores the DTA/Timestamp i8 equivalence will run into this problem.

I think you're correct. On your branch


```python
In [32]: df = pd.DataFrame({"A": pd.date_range('2000', periods=2, tz="US/Central")})

In [33]: df
Out[33]:
                          A
0 2000-01-01 00:00:00-06:00
1 2000-01-02 00:00:00-06:00

In [34]: pa.Table.from_pandas(df).to_pandas()
Out[34]:
                          A
0 2000-01-01 06:00:00-06:00
1 2000-01-02 06:00:00-06:00
```

Which seems strange to me... Why would us changing back to the old way break pyarrow? I'll do some digging.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24623?src=pr&el=h1) Report
> Merging [#24623](https://codecov.io/gh/pandas-dev/pandas/pull/24623?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/dc91f4cb03208889b98dc29c1a1fe46b979e81c7?src=pr&el=desc) will **decrease** coverage by `49.34%`.
> The diff coverage is `83.33%`.

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

```diff
@@             Coverage Diff             @@
##           master   #24623       +/-   ##
===========================================
- Coverage   92.37%   43.03%   -49.35%     
===========================================
  Files         166      166               
  Lines       52384    52368       -16     
===========================================
- Hits        48390    22536    -25854     
- Misses       3994    29832    +25838
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `43.03% <83.33%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24623?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/packers.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhY2tlcnMucHk=) | `14.65% <0%> (-73.57%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `35.74% <100%> (-61.19%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `48.45% <100%> (-47.81%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `52.71% <50%> (-41.6%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `66.21% <87.5%> (-31.48%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [128 more](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24623?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/24623?src=pr&el=footer). Last update [dc91f4c...a46a2cc](https://codecov.io/gh/pandas-dev/pandas/pull/24623?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/24623?src=pr&el=h1) Report
> Merging [#24623](https://codecov.io/gh/pandas-dev/pandas/pull/24623?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/46a31c9944a2f6c25282f65a8bf8247abe848bc9?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #24623      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52327    52293      -34     
==========================================
- Hits        48343    48312      -31     
+ Misses       3984     3981       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (-0.01%)` | :arrow_down: |
| #single | `43.07% <86.2%> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24623?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/packers.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhY2tlcnMucHk=) | `88.21% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `97.67% <100%> (ø)` | :arrow_up: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `94.3% <100%> (ø)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.92% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.44% <100%> (+0.16%)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `97.81% <100%> (+0.09%)` | :arrow_up: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `87.86% <0%> (-0.24%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24623/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `90.25% <0%> (+0.02%)` | :arrow_up: |

------

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

@jbrockmendel this needs additional tests?
> this needs additional tests?

Yes.  Waiting to get weigh-in from @TomAugspurger @jorisvandenbossche @mroeschke.
I've been out of the loop with DatetimeArray development, but overall I am +1 with the goals outlined in https://github.com/pandas-dev/pandas/issues/24559#issuecomment-451789361

It would be nice if there was 1 entry point to parse all datetime values, but given that we have so many constructors already; this PR's step of maintaining consistency with the others is good.
> I am personally -1 on adding yet another constructor that can parse datetime-like values.

@jorisvandenbossche can you clarify this objection? You would like the following to raise, as it does on master?

```python
In [6]: pd.arrays.DatetimeArray(np.array(['2015-01-01']))
Out[6]:
<DatetimeArray>
['2015-01-01 00:00:00']
Length: 1, dtype: datetime64[ns]
```
@jbrockmendel can you rebase
> can you clarify this objection?

Yes, I would prefer the situation as it is on master. 
I would strongly prefer to keep the main DatetimeArray constructor simple: only accepting the actual values, and nothing more. (with some exceptions of course, like unboxing containers (Series/Index) that contain such values)

Reasons for my current thinking on that:

- It's not needed. We already have the user-facing `pd.array` entry-point, and `to_datetime` for custom string parsing. 
  If we add similar ability of `pd.array` and `to_datetime` to `DatetimeArray` as well, users will start using it for that, and it is just yet another general constructor that we have to maintain

- Keeping the range of accepted values limited for DatetimeArray gives clearer and more explicit code. If you read this in code, you can know that the values are already an array of datetime64 values. If have something else (scalars, strings, ..), then you can use the specific functionality to deal with those. 
  (although on this aspect: we are still mixing integers and datetimes, and also giving it different behaviour, which already is a bit annoying)

It's basically what we have been discussing regularly before (in https://github.com/pandas-dev/pandas/issues/23212, and probably some other places that I don't directly recall), and what we have done for IntegerArray.
Thanks. There's a 3rd (less important) argument: performance. The slowdown in https://github.com/pandas-dev/pandas/pull/24666 comes down to using `sequence_to_td64ns`. 
https://github.com/pandas-dev/pandas/pull/24666#issuecomment-452331613. That can be resolved, but at the cost of additional code complexity.
This PR is on hold pending discussion in #24559.

There are parts of this PR that I expect to remain unchanged regardless of how that is resolved:
- going through sequence_to_dt64ns rather than re-implementing a bunch of code
  - crippling the public constructor can be done in the block that disallows bool dtype; just disallow a bunch more stuff in that same line.
- doing freq validation and never letting the public constructor return an invalid instance
  - yes I know that has perf implications.  That is why there is a _private_ constructor _simple_new that doesn't do validation
@jbrockmendel the performance in 
https://github.com/pandas-dev/pandas/pull/24666#issuecomment-452331613 is unrelated to freq validation.

> going through sequence_to_dt64ns rather than re-implementing a bunch of code

that's fine by me, as long as there's a fastpath when the input is known to be an ndarray, Datetime/TimedeltaArray (or box around those) with the correct dtype. 
Are we holding the RC for this? I haven't followed too closely, but IIRC there's two issues

1. `DTA.__init__` has (incidentally) become more flexible as a result of this PR. That's relatively easy to fix by being bit stricter in the `__init__`
2. The handling of the i8 values?

Where are we on number 2? Do we have agreement on the desired behavior, and does this PR implement that?
On the desired behavior (let's keep the discussion here now that the PR is open):

https://github.com/pandas-dev/pandas/issues/24559#issuecomment-452075476 has a nice summary.

@jorisvandenbossche prefers 

1. Reverting the change to `DTI.__new__` for passing integers. Add a warning that it'll change in the future.
2. Keep `DTA.__init__`  as it is now (same as Timestamp IIRC)

I think that's my preference as well, if I understand things correctly. @jbrockmendel do you need / want any help pushing this across the line? I'm busy with there things most of today, but can carve out some time if needed. It'd be good to do the RC today I think.
As far as I understand, this PR is not related to the handling of integers (at least that is what I understood from asking @jbrockmendel about it), so I would prefer to keep the discussion about that in the issue.
About this PR, if we want the changes of this PR in general, it's indeed easy to defer the discussion about the flexibility of the constructor by simply adding a check if dtype is M8 or int, and otherwise raising an error.

But I thought there was still disagreement about whether we want this freq validation or not? (because of the performance hit) Or is that resolved?
> As far as I understand, this PR is not related to the handling of integers

One thing that it does, though (from https://github.com/pandas-dev/pandas/issues/24559#issuecomment-451789361) is changing the behaviour of DatetimeArray. But so not the reverting of DatetimeIndex + adding a warning there. I would personally keep that for a separate PR, to keep it distinct from the unrelated discussion here (eg flexibility of constructor, freq validation)
I propose to do the following:

- rebase
- revert parts of this PR that affect dtype-treatment, leaving only parts that:
    - ensure freq is validated, so the public DTA constructor never gives an invalid instance
    - retains a private fastpath _simple_new for DTA
    - closes #24615
- address concerns about the DTI 0.23.4/master behavior separately

Everyone on board?
Not fully on board, that still changes the behaviour of DatetimeArray regarding M8 input, which is something that first should be decided upon (see the table I added in https://github.com/pandas-dev/pandas/issues/24559, writing yet another comment with possible options)
> Not fully on board, that still changes the behaviour of DatetimeArray regarding M8 input,

I'm specifically proposing to revert changes to the behavior of DatetimeArray with regard to M8 input.

edit: "revert" as in revert changes made in an earlier version of this PR.
But so without using `_from_seqence` then? As that does handle M8 differently
> But so without using _from_seqence then? As that does handle M8 differently

The suggested game-plan didn't discuss how to get from here to there.  Just that the DTA behavior would revert to how it in master.
Hello @jbrockmendel! Thanks for updating the PR.



















Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on January 09, 2019 at 17:54 Hours UTC
ATM I'm trying to put together a branch that _only_ adds freq validation.  De-duplicating code, making _from_sequence match `__init__`, worrying about DTI... later.
Can't you move the freq validation of `_from_sequence` to a separate helper function(s), and call that in `__init__` so you don't need to call `_from_sequence`/`sequence_to_dt64ns` in the init? That might be much simpler?
(that comment was about the current diff, not about your last comment, to be clear)
> Can't you move the freq validation of _from_sequence to a separate helper function(s)

That's close to what I'm doing in the other branch I mentioned.  It's a little inelegant to do as a helper function since it needs to do a bit at the beginning and a bit at the end.  But It's only 3 lines in the new branch, so I'm not going to sweat it for now.
See #24686.