WIP: make _holder changeover
State: closed

cc @jreback @TomAugspurger 

changes _holder from DTI/TDI to DTA/TDA, pretty much all the diff from blocks.internals and core.dtypes

Not yet green locally, but I'm presenting this as a viable alternative to the all-at-once still in #24024

Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
Haven't looked here yet, but my vote it to stop splitting things off. I
have some followups to the other PR that I'd like to get to that are
currently blocked.

On Tue, Jan 1, 2019 at 5:21 PM jbrockmendel <notifications@github.com>
wrote:

> cc @jreback <https://github.com/jreback> @TomAugspurger
> <https://github.com/TomAugspurger>
>
> changes _holder from DTI/TDI to DTA/TDA, pretty much all the diff from
> blocks.internals and core.dtypes
>
> Not yet green locally, but I'm presenting this as a viable alternative to
> the all-at-once still in #24024
> <https://github.com/pandas-dev/pandas/pull/24024>
> ------------------------------
> You can view, comment on, or merge this pull request online at:
>
>   https://github.com/pandas-dev/pandas/pull/24540
> Commit Summary
>
>    - make _holder changeover
>
> File Changes
>
>    - *M* pandas/core/dtypes/common.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-0> (5)
>    - *M* pandas/core/dtypes/concat.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-1> (14)
>    - *M* pandas/core/dtypes/dtypes.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-2> (16)
>    - *M* pandas/core/dtypes/generic.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-3> (2)
>    - *M* pandas/core/internals/blocks.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-4> (201)
>    - *M* pandas/tests/dtypes/test_dtypes.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-5> (17)
>    - *M* pandas/tests/extension/test_common.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-6> (8)
>    - *M* pandas/tests/internals/test_internals.py
>    <https://github.com/pandas-dev/pandas/pull/24540/files#diff-7> (18)
>
> Patch Links:
>
>    - https://github.com/pandas-dev/pandas/pull/24540.patch
>    - https://github.com/pandas-dev/pandas/pull/24540.diff
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIoDb4LM389Hpsq_H6DxY2WhgSB2lks5u--1lgaJpZM4ZmEUK>
> .
>

I hear where you're coming from (and am definitely ready to spend some time away from DTA/TDA), but I don't see a resolution to the constructor discussion, and that's a blocker for me on #24024.
An actual blocker, as in you'll -1 the merge until it's addressed?

FYI, I think the handling of i8values has been addressed. Working on freq
validation now.

On Tue, Jan 1, 2019 at 5:27 PM jbrockmendel <notifications@github.com>
wrote:

> I hear where you're coming from (and am definitely ready to spend some
> time away from DTA/TDA), but I don't see a resolution to the constructor
> discussion, and that's a blocker for me on #24024
> <https://github.com/pandas-dev/pandas/pull/24024>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450765094>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHItV-LUOO6fZ_oSO5pn4d3uByEEteks5u--7OgaJpZM4ZmEUK>
> .
>

> An actual blocker, as in you'll -1 the merge until it's addressed?

I know I'm not the one who needs to be convinced, but yah.

> FYI, I think the handling of i8values has been addressed. Working on freq
validation now.

That would totally get me to stop complaining!
Any pandas maintainer can block a PR from being merged with a -1.

On Tue, Jan 1, 2019 at 5:31 PM jbrockmendel <notifications@github.com>
wrote:

> An actual blocker, as in you'll -1 the merge until it's addressed?
>
> I know I'm not the one who needs to be convinced, but yah.
>
> FYI, I think the handling of i8values has been addressed. Working on freq
> validation now.
>
> That would totally get me to stop complaining!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450765296>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIkZhJTMDBn-Icqq6eulvauPCIzb_ks5u--_GgaJpZM4ZmEUK>
> .
>

well, mostly.  There should still be a private, inference-free, validation-free constructor.  We could call it "easy_init"
I don't think that's possible unless we use `__new__`, and I don't want the
complexity that entails.

On Tue, Jan 1, 2019 at 5:36 PM jbrockmendel <notifications@github.com>
wrote:

> well, mostly. There should still be a private, inference-free,
> validation-free constructor. We could call it "easy_init"
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450765578>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIvN51HeGWDezTit74THPRDFYJGUbks5u-_EIgaJpZM4ZmEUK>
> .
>

I've gotten the test failures down to almost entirely pickle-based.  @TomAugspurger IIRC you had to solve that in the other branch.  Was there a trick there?
Define `DatetimeArray.__init__` ;)
> Define DatetimeArray.__init__ ;)

Yep, that did it.  And plays nicely with _simple_new to boot.
> well, mostly. There should still be a private, inference-free, validation-free constructor. We could call it "easy_init"

Why can't we simply make the DTA (et all) ``__init__`` just be unfriendly if you don't pass accordning the guarantees; yes they will be public but will have very constrained inputs. ?
That's basically what we do on #24024. The permitted types for data

* ndarray[i8]
* ndarray[datetime64[ns]]
* DatetimeArray
* Series / Index of the above

This means that `__init__` is always no-copy (unless explicitly requested
via the `copy` param). At most we do a view or an unboxing.

On Tue, Jan 1, 2019 at 6:26 PM Jeff Reback <notifications@github.com> wrote:

> well, mostly. There should still be a private, inference-free,
> validation-free constructor. We could call it "easy_init"
>
> Why can't we simply make the DTA (et all) __init__ just be unfriendly if
> you don't pass accordning the guarantees; yes they will be public but will
> have very constrained inputs. ?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450768138>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIvjKFuUmew9ydrhB0zd2iabfvLIpks5u-_yvgaJpZM4ZmEUK>
> .
>

> Why can't we simply make the DTA (et all) __init__ just be unfriendly if you don't pass accordning the guarantees; yes they will be public but will have very constrained inputs. ?

My immediate claim is that this discussion is large enough that it should be separated out from #24024.  That is the purpose of _this_ PR.

The answer to the question: because it's public!  because people will use it!  because having it behave differently from `DTI.__new__` is confusing!  becuase having it fail to do `freq` validation* means users are going to `shift` themselves to absurd outputs!

*possibly out of date, but even if so, there should be non-validating (performant) _private_ constructor!
ok, I am fine with exploring alternative ways of doing this. But at this point, would be more productive to merge #24024 yes?
> But at this point, would be more productive to merge #24024 yes?

If the concensus is that smaller bits-for-better-review is no longer worthwhile, then we can close this and I'll do one more mondo pass on 24024.

That said, the only tests that are still failing here are in test_packers, so if we can fix those and do this indepndently, we cut 40% off the remaining diff in 24024
@jbrockmendel https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450770508

ok let's do that (merge this), then rebase and merge #24024 
> ok let's do that (merge this)

I like the attitude, but a lot of the point of smaller pieces is making it feasible to fine-tooth-comb it.
So it looks like the packers tests are resolved if I change `DatetimeArray.__init__` to treat `M8[ns]` values like int64 values, which I'd really rather not do.  Any thoughts on another way to resolve this?
I'm looking into this right now. I'd going to try to crack it tonight, but
I'm pretty confused at the moment. I'll post if I give up for the night.

On Tue, Jan 1, 2019 at 9:31 PM jbrockmendel <notifications@github.com>
wrote:

> So it looks like the packers tests are resolved if I change
> DatetimeArray.__init__ to treat M8[ns] values like int64 values, which
> I'd really rather not do. Any thoughts on another way to resolve this?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450781886>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHImc6epNMD2IN_jhpbxdqwfZL5K-Aks5u_CgngaJpZM4ZmEUK>
> .
>

A `pd.date_range` apparently calls `DatetimeArray.__init__` four times,
which I hope isn't necessary.

On Tue, Jan 1, 2019 at 9:35 PM Tom Augspurger <tom.augspurger88@gmail.com>
wrote:

> I'm looking into this right now. I'd going to try to crack it tonight, but
> I'm pretty confused at the moment. I'll post if I give up for the night.
>
> On Tue, Jan 1, 2019 at 9:31 PM jbrockmendel <notifications@github.com>
> wrote:
>
>> So it looks like the packers tests are resolved if I change
>> DatetimeArray.__init__ to treat M8[ns] values like int64 values, which
>> I'd really rather not do. Any thoughts on another way to resolve this?
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450781886>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/ABQHImc6epNMD2IN_jhpbxdqwfZL5K-Aks5u_CgngaJpZM4ZmEUK>
>> .
>>
>

Making progress I think, but giving up for the night.

On Tue, Jan 1, 2019 at 9:36 PM Tom Augspurger <tom.augspurger88@gmail.com>
wrote:

> A `pd.date_range` apparently calls `DatetimeArray.__init__` four times,
> which I hope isn't necessary.
>
> On Tue, Jan 1, 2019 at 9:35 PM Tom Augspurger <tom.augspurger88@gmail.com>
> wrote:
>
>> I'm looking into this right now. I'd going to try to crack it tonight,
>> but I'm pretty confused at the moment. I'll post if I give up for the night.
>>
>> On Tue, Jan 1, 2019 at 9:31 PM jbrockmendel <notifications@github.com>
>> wrote:
>>
>>> So it looks like the packers tests are resolved if I change
>>> DatetimeArray.__init__ to treat M8[ns] values like int64 values, which
>>> I'd really rather not do. Any thoughts on another way to resolve this?
>>>
>>> —
>>> You are receiving this because you were mentioned.
>>> Reply to this email directly, view it on GitHub
>>> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450781886>,
>>> or mute the thread
>>> <https://github.com/notifications/unsubscribe-auth/ABQHImc6epNMD2IN_jhpbxdqwfZL5K-Aks5u_CgngaJpZM4ZmEUK>
>>> .
>>>
>>

That hasn't been implemented yet.

On Wed, Jan 2, 2019 at 8:59 AM jbrockmendel <notifications@github.com>
wrote:

> *@jbrockmendel* commented on this pull request.
> ------------------------------
>
> In pandas/core/arrays/datetimes.py
> <https://github.com/pandas-dev/pandas/pull/24540#discussion_r244757098>:
>
> > @@ -1568,6 +1573,8 @@ def sequence_to_dt64ns(data, dtype=None, copy=False,
>          copy = False
>      elif isinstance(data, ABCSeries):
>          data = data._values
> +    elif isinstance(data, ABCPandasArray):
>
> i seem to recall something about extension dtypes being registered in such
> a way that infer_dtype could pick up on them. will take a look
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#discussion_r244757098>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIq87X1_H0AnFsk70-P372E_o3Bapks5u_MlWgaJpZM4ZmEUK>
> .
>

What is currently the status of this PR in relation to https://github.com/pandas-dev/pandas/pull/24024 ?
> What is currently the status of this PR in relation to #24024 ?

Many of the comments here need to be addressed in 24024 as well, but on its own this PR is dead in the water since I can't get the packers tests passing.
Sounds like this will be a decent followup then, since the packers tests
will be fixed.

On Wed, Jan 2, 2019 at 11:02 AM jbrockmendel <notifications@github.com>
wrote:

> What is currently the status of this PR in relation to #24024
> <https://github.com/pandas-dev/pandas/pull/24024> ?
>
> Many of the comments here need to be addressed in 24024 as well, but on
> its own this PR is dead in the water since I can't get the packers tests
> passing.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24540#issuecomment-450921211>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIm6oL0NsQ9wIobpik4QQH9CL7cHqks5u_OZBgaJpZM4ZmEUK>
> .
>

> Many of the comments here need to be addressed in 24024 as well,

@jbrockmendel were they addressed in the end? 
> were they addressed in the end?

I'll double-check.  Still have a browser tab open trying to keep track of which issues from 24024 are still open.  Will see if one of the follow-up Issues can serve as a tracker.