EA: support basic 2D operations
State: closed

This is one of (I think) two things we will actually require of EA authors.  The other is we will need something like view to return a new EA object with a view on the same data.  That will be implemented in a separate PR following discussion.

cc @jreback @jorisvandenbossche @TomAugspurger 

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

Comments:
Remind me, why is this necessary? Is this for the reshape mixin? This will be a breaking change for EA authors. 
> Remind me, why is this necessary? Is this for the reshape mixin? This will be a breaking change for EA authors.

Yes.  It is a breaking change, but it is also much more robust than the other options (e.g. metaclass) which I found to be fragile MRO-wise.
Updated to include parts of #27153, the upshot of which is that this now includes *all* the expected breaking changes for EA authors.

- EAs must implement `view` that returns a new object viewing the same data
- EAs that don't natively support 2D need to apply a `implement_2d` decorator.
    - Unlike in the original version of this PR, they do NOT need to implement shape, can stick with `__len__`.

Other breaking changes that aren't EA-author-specific:
- Categorical.ravel() now returns Categorical instead of ndarray
- DTA.view(), TDA.view(), PA.view() return DTA/TDA/PA instead of ndarray
can you update the title of the PR to better reflect things
@jbrockmendel is this the PR to focus on for 2D EA stuff?

Is there anything you definitely want to have in the release / release candidate?
> is this the PR to focus on for 2D EA stuff?

Yes.

> Is there anything you definitely want to have in the release / release candidate?

If we're going to do a deprecation cycle for the behavior of Categorical.ravel, I'd like to get that in.
> >  is this the PR to focus on for 2D EA stuff?
>
> Yes.

I am a bit lost in the different PRs / POC you made. 
Can you explain how this relates to the other PRs? How does this differ and why did you decide to take this path (compared to the other PRs).
> I am a bit lost in the different PRs / POC you made.

Fair enough.  Quick summary:
 - #26914, #26954, #27015 were all proof-of-concept PRs, now closed.
 - #27153 was to implement view, T, and ravel, but I decided to roll that into this PR, so that is now closed.
 - #27199 breaks off the `ravel()` part of this PR, deprecating the behavior of Categorical.ravel instead of changing it immediately.  That is still open.  I am expecting/hoping that will be merged after which I will rebase this.

> How does this differ and why did you decide to take this path (compared to the other PRs).

The approach for this PR centers around two goals:
 - Keep the scope limited (self-explanatory)
 - Minimize what we ask of downstream authors

We can patch most of the relevant methods of a 1D-only EA using a decorator, here implemented as `implement_2d` (note ATM this decorator only patches shape, len, size.  In follow-ups it will also patch take, getitem, setitem).

One thing that AFAICT we can't patch and do need the EA authors to implement is `view` (I'm open to other names) to get a new object with the same underlying data.  This is needed for `reshape`, `T`, and `ravel`.

Finally, the `_allows_2d` attribute is there so authors can say "No, don't patch anything, I've already implemented 2D support".
> We can patch most of the relevant methods of a 1D-only EA using a decorator

What do you mean with "we" here? Can pandas apply this decorator to external EAs, or need those external packages do that themselves?

How does this compare to the composition POC ?
> What do you mean with "we" here? Can pandas apply this decorator to external EAs, or need those external packages do that themselves?

Three options come to mind:
- ask EA authors apply it themselves (best)
- use a metaclass to apply it automatically
- apply it ourselves to any unrecognized EA we see

Asking EA authors to apply it is the most transparent option, so is my preference.

> How does this compare to the composition POC ?

The composition POC required fewer edits on the part of EA authors, but required compatibility code within pandas to unwrap composited EAs.
> apply it ourselves to any unrecognized EA we see

Do you think this is possible? Because we typically see an instance, not the class itself? 
Or, that sounds then more as the composition approach.

> Asking EA authors to apply it is the most transparent option, so is my preference.

IMO that is very difficult to do in practice if you want some compatibility with different version (to avoid that the latest pandas only works with latest external package, needing such exact pins makes assembling environments always harder)

> The composition POC required fewer edits on the part of EA authors, but required compatibility code within pandas to unwrap composited EAs.

As you can probably guess, to me this sounds as a plus for the composition appraoch.
> Do you think this is possible? Because we typically see an instance, not the class itself?

I'm imagining something like:

```
if not ea._supports_2d:
    # maybe issue a warning?
    implement_2d(type(ea))
    implement_2d(ea)
```

Patching a user's array is not ideal, which is why the approach in this PR is to ask EA authors to do it themselves.

> IMO that is very difficult to do in practice if you want some compatibility with different version

Yes.  One option is to provide a dummy decorator in 0.25.0 and issue a warning asking authors to apply it (and implement `view`).  Then actually require it in 0.26.0 and hopefully the word will have gotten out by then.  We can probably fall back to the patch-it-ourselves approach to avoid complete incompatibility.

>> The composition POC required fewer edits on the part of EA authors, but required compatibility code within pandas to unwrap composited EAs.

> As you can probably guess, to me this sounds as a plus for the composition appraoch.

Take a look at the diff in #27015 to get an idea how much compat code this requires on our end.
Any thoughts here?  The next step to add would be patching setitem/getitem/take/iter, but that would make for a much larger diff (especially once tests are added) so I'd like to get a round of discussion in before that
I would keep EA strictly 1d

i still don’t understand why that would not be simpler 
and it’s much less confusing for EA

if there are reshapes needed then ExtnsionBlock is the place for this


> I would keep EA strictly 1d
> i still don’t understand why that would not be simpler
> and it’s much less confusing for EA

Jeff you're like the brother I never had* but you're all over the place.  This is the exact opposite of the opinion you expressed before leaving the sprint.

\* Don't tell my brother I said this.
> Patching a user's array is not ideal, which is why the approach in this PR is to ask EA authors to do it themselves.

Not ideal, but possible?


> > As you can probably guess, to me this sounds as a plus for the composition appraoch.
>
> Take a look at the diff in #27015 to get an idea how much compat code this requires on our end.

Yes, but then that's what we have to live with if we want to go that route? (for not requiring changes for EAs) In the end it is a trade-off we need to make: is that additional compat code worth the simplification in other parts of the internals? 
I suppose that the patching of the user's array or the composition POC would end up rather similarly?

I am still strongly in favor of trying to find a solution that does not need changes for EAs.  (and as I also mentioned before, I would prefer to see our internals EAs to be treated the same, but that of course basically means they would need to stay 1D as well, and all EAs use that compat code)


@jbrockmendel To maybe go back to the root of the discussion / misunderstandings, could you point to one (or a few) examples of 1D/2D complexities in the internals or in code reaching into the internals ?
> > I would keep EA strictly 1d
> > i still don’t understand why that would not be simpler
> > and it’s much less confusing for EA
> 
> Jeff you're like the brother I never had* but you're all over the place. This is the exact opposite of the opinion you expressed before leaving the sprint.
> 
> * Don't tell my brother I said this.

@jbrockmendel I *never actually liked*  support for 2D EA, just thru it out as a *possible* solution. We need to only support 1D.
>> Patching a user's array is not ideal, which is why the approach in this PR is to ask EA authors to do it themselves.

> Not ideal, but possible?

I believe so, yes.

>  I suppose that the patching of the user's array or the composition POC would end up rather similarly?

I'm not sure I understand this sentence.  Can you please clarify?

Whether by patching the user's array for them or requiring authors to apply the decorator themselves, the end result is that we can rip out effectively all the compat kludges.  With the composition approach we get rid of some of the compat code, but still have to have unwrapping code all over the place.

> could you point to one (or a few) examples of 1D/2D complexities in the internals or in code reaching into the internals ?

- Basically all of NonConsolidateableMixin and large parts of ExtensionBlock exist to hide the fact that block.shape != block.values.shape
- Have to special-case to avoid a call to `ravel`: https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals/blocks.py#L617 then below on 639 special case again for `reshape`
- Block constructor passes around `ndim`, would be unnecessary with 2D-column-like EA.
- BlockManager._verify_integrity would be trivial
- This workaround was a PITA: https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals/managers.py#L481
- Special casing for non-reshapeable: https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals/managers.py#L1045
- special casing buried in io.sql: https://github.com/pandas-dev/pandas/pull/27254/files?file-filters%5B%5D=.py#diff-b41f9fd042c423682f8e4c4d808dbe64L703

Or as @jreback put it [earlier](https://github.com/pandas-dev/pandas/issues/24583#issuecomment-451151591) this year:
```
If we have an array class then it needs to be almost a drop in for numpy arrays. Not having basic reshaping is causes lots of headaches.
```
Do we need to have another call to sync up here?

@jreback can you expand on https://github.com/pandas-dev/pandas/pull/27142#issuecomment-510296916 a bit? IIUC, that would mean we can still have `Block.shape != Block.values.shape` for extension blocks, right?
yes we should probably have a call on this


This week I'm pretty flexible except for Friday morning.  Next week TWF are all clear.  (UTC-7)
I can do a call today other than 11:00-11:30 Central. Otherwise next week on Tuesday.
next week

I can be reached on my cell 917-971-6387

> On Jul 18, 2019, at 9:21 AM, Tom Augspurger <notifications@github.com> wrote:
> 
> I can do a call today other than 11:00-11:30 Central. Otherwise next week on Tuesday.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

If it is only next week, it could also be monday week after, then I can
join (but of course feel free to already have a chat before)

Op do 18 jul. 2019 11:26 schreef Jeff Reback <notifications@github.com>:

> next week
>
> I can be reached on my cell 917-971-6387
>
> > On Jul 18, 2019, at 9:21 AM, Tom Augspurger <notifications@github.com>
> wrote:
> >
> > I can do a call today other than 11:00-11:30 Central. Otherwise next
> week on Tuesday.
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub, or mute the thread.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27142?email_source=notifications&email_token=AAHZEUC2OIV6FYGHK47SLWLQACDUBA5CNFSM4H4MWHG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2I3GUI#issuecomment-512865105>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAHZEUEE44TASW3GOFKHATLQACDUBANCNFSM4H4MWHGQ>
> .
>

@jreback @jbrockmendel are you free for a call this afternoon? I'm free any time after ~1:00 central.


i would be available at 4pm est or after
or thurs / friday

> On Jul 23, 2019, at 1:01 PM, Tom Augspurger <notifications@github.com> wrote:
> 
> @jreback @jbrockmendel are you free for a call this afternoon? I'm free any time after ~1:00 central.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

Today after 4 EST works for me.
Let's do 4:00 EST / 3:00 Central / 1:00 Pacific then. meet.google.com/bxs-pjaf-oqi
@TomAugspurger what is the google link?