API: consistent __array__ for datetime-like ExtensionArrays
State: closed

closes #23569
 
Already makes the `__array__` for the datetimelike EAs consistent with each other and with Series/Index (for the default case of not specifying a dtype)



Labels:
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

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













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

##### Comment last updated on January 05, 2019 at 20:26 Hours UTC
isort failure: 

```
Check import format using isort
ERROR: /home/travis/build/pandas-dev/pandas/pandas/tests/extension/base/interface.py Imports are incorrectly sorted.
```
Yeah, I will still need to get used to it :-) 
Do you have it as a commit hook?
I have for dask-ml. I'm experimenting with it in pandas today.

1. install pre-commit
2. paste this to `.pre-commit-config.yaml`

```
-   repo: https://github.com/pre-commit/mirrors-isort
    rev: 'f35773e46d096de5c45365f1a47eeeef36fc83ed'  # Use the revision sha / tag you want to point at
    hooks:
    -   id: isort

-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v2.0.0  # Use the ref you want to point at
    hooks:
    -   id: flake8
```

3. run pre-commit install


I'll open an issue suggesting that for the docs if it works well.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23593?src=pr&el=h1) Report
> Merging [#23593](https://codecov.io/gh/pandas-dev/pandas/pull/23593?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f074abef5b3b59d072cc4568949673a39bf58dd1?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/23593/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23593?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23593      +/-   ##
==========================================
+ Coverage   92.37%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52379    52377       -2     
==========================================
  Hits        48386    48386              
+ Misses       3993     3991       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (ø)` | :arrow_up: |
| #single | `43.01% <77.77%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23593?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23593/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `88.24% <ø> (+0.05%)` | :arrow_up: |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/23593/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `97.86% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/23593/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `98.54% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23593/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `97.84% <100%> (-0.01%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/23593/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.09% <0%> (+0.09%)` | :arrow_up: |

------

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

Implement view while at it?
Wrong button, woops
> Implement view while at it?

On EAs? Is there a reason to do that?
> On EAs? Is there a reason to do that?

IIRC from other branches we'll need to do it eventually.  Since I don't have the precise reason on hand, it's reasonable to wait until actually needed.
From https://github.com/pandas-dev/pandas/issues/23569#issuecomment-437310547

You questioned whether we should  implement `np.array(period_array, dtype=int)`. You haven't done that right?

I'm not really clear what's best here. Do we publicly document exactly what an ordinal is, beyond just saying that it's an integer (though maybe that's enough justify making `int`)?
@jbrockmendel can you rebase this.
This one is from Joris :)
ok then @jorisvandenbossche lgtm.
@jorisvandenbossche can you rebase
Fixed the linting error.

But, we still need to decide on https://github.com/pandas-dev/pandas/pull/23593#issuecomment-437520571: should `np.array(period_array, dtype='int')` work? 
PeriodIndex currently does this (but also without making a copy, which I think we should change to conform to numpy behaviour).

> I'm not really clear what's best here. Do we publicly document exactly what an ordinal is, beyond just saying that it's an integer (though maybe that's enough justify making int)?

I don't really care that much. If "power users" want to work with the integers, I think the `asi8` attribute is more explicit to use? (and is already public)
Sounds good to me.

On Fri, Nov 23, 2018 at 4:53 AM Joris Van den Bossche <
notifications@github.com> wrote:

> Fixed the linting error.
>
> But, we still need to decide on #23593 (comment)
> <https://github.com/pandas-dev/pandas/pull/23593#issuecomment-437520571>:
> should np.array(period_array, dtype='int') work?
> PeriodIndex currently does this (but also without making a copy, which I
> think we should change to conform to numpy behaviour).
>
> I'm not really clear what's best here. Do we publicly document exactly
> what an ordinal is, beyond just saying that it's an integer (though maybe
> that's enough justify making int)?
>
> I don't really care that much. If "power users" want to work with the
> integers, I think the asi8 attribute is more explicit to use? (and is
> already public)
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/23593#issuecomment-441210164>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHItg6Sd05Rh7_p7EkVCv2N2HuOThlks5ux9OlgaJpZM4YWVyz>
> .
>

yeah this is ok 
looking at this again. I would be happy to simply ignore the passed ``dtype`` for PeriodIndex, as the only other places we handle the dtype is actually for ``object`` dtype itself. I think we need comprehensive handling, adding bits and pieces is pretty odd here. 

maybe better to simply do ``self.astype(dtype)`` if dtype is not None, then call ``__array__`` on the result w/o the dtype, as we already handle this property.
>  I think we need comprehensive handling, adding bits and pieces is pretty odd here.

I'm not sure I follow. `__array__` is called by things like `np.asarray`, so we need to handle `dtype` there. Unless, are you suggesting that we call `PeriodArray.astype` from within `PeriodArray.__array__`?
> 'm not sure I follow. __array__ is called by things like np.asarray, so we need to handle dtype there. Unless, are you suggesting that we call PeriodArray.astype from within PeriodArray.__array__?

no the issue is we don't comprehensively handle dtype at all.

I think a better strategy is to have something like this, which is quite general

```
def __array__(self, dtype=None):
    if dtype is not None:
        self = self.astype(dtype, copy=False)
    return self._data
```

I don't think that'll quite work, since the return type of `DatetimeLikeArray.astype` is `Union[Extension, ndarray]`. So we wouldn't know if we need to (or can) get a `._data` attribute from the result of `self.astype`.

Though, there's a requirement that `dtype` be a NumPy dtype right? So presumable we could call `np.dtype(dtype)`, which will raise if the type isn't OK (like `np.asarray(datetime_array, dtype='category')`.)
> Though, there's a requirement that dtype be a NumPy dtype right? So presumable we could call np.dtype(dtype), which will raise if the type isn't OK (like np.asarray(datetime_array, dtype='category').)

From a quick test, this already raises from numpy before it reaches the pandas code

> maybe better to simply do self.astype(dtype) if dtype is not None, then call __array__ on the result w/o the dtype, as we already handle this property.

Currently, we use `np.asarray(self, dtype=dtype)` in the `astype` implementation of PeriodArray, so that would need to be changed (eg for `np.array(period_array, dtype=str)`, that would a recursion error. While on this branch, that works (by letting numpy deal with converting the periods to strings).
@TomAugspurger how does this one relate to the DatetimeArray PR? Do we still want to merge this, or are almost all changes already in that PR as well?
It's not clear to me right now. I think I held off changing `DatetimeArray.__array__` in that PR.

I have a WIP branch that changes `DatetimeArray.__array__` warn for TZ-aware data that in the future it'll return an ndarray of Timestamp objects.  That'll require some light modifications.

I think at the very least the tests here would be helpful.
> DatetimeArray.__array__ warn for TZ-aware data that in the future it'll return an ndarray of Timestamp objects.

Why is that needed? For DatetimeArray we can directly do the 'right' thing? (or do you mean for Index/Series ?)
Possibly. That would require duplicating the warning checking logic though I think, at least between Series, DatetimeIndex, and DatetimeTZBlock. Doing it at the lowest place seemed most sensible (even if it means introducing DatetimeArray to the world with a  deprecation warning).
Yeah, that's true. 

For this PR, is it fine to merge this, or would you rather want one with only the tests? (although I think they will fail without the other changes)
Fine with merging this first. I don't *think* it'll cause any issues. the only thing I added to `DatetimeArray.__array__` was another TODO.

During / after #24024, will we be able to fix the TODO in

https://github.com/pandas-dev/pandas/pull/23593/files#diff-26a6d2ca7adfca586aabbb1c9dd8bf36R55 by calling `return np.asarray(self._data, dtype=dtype)`? Is the only thing blocking that the inheritance?
OK, I fixed the merge conflicts here, so pending my answers to Jeff's comments, this can be merged I think
FYI, another merge conflict is present, sorry.

Regarding your comment in https://github.com/pandas-dev/pandas/pull/23593#issuecomment-447049239 about putting the warning on Series / Index, instead of DatetimeArray, you were right :) Doing it that was was much less invasive (though still not a tiny change). https://github.com/TomAugspurger/pandas/commit/63b7ad8e0d0cc1b720dce227c2d69305802c6dea has the diff on top of #24024. I'll open a PR once it's in.

With respect to this PR. I do need to define `DatetimeArray.__array__` to check whether the data is TZ aware or not. That would mean that just TimedeltaArra inherits the base `__array__`, though I could change my implementation to use `super()` for tz-naive, so I think your approach is fine.
need to merge master on this