EA: implement+test EA.view
State: closed

Broken off from #27142, plus some type annotations

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

Comments:
SGTM.

On Mon, Jul 29, 2019 at 2:12 PM jbrockmendel <notifications@github.com>
wrote:

> *@jbrockmendel* commented on this pull request.
> ------------------------------
>
> In pandas/core/arrays/base.py
> <https://github.com/pandas-dev/pandas/pull/27633#discussion_r308392321>:
>
> > @@ -867,6 +867,24 @@ def copy(self) -> ABCExtensionArray:
>          """
>          raise AbstractMethodError(self)
>
> +    def view(self, dtype=None) -> ABCExtensionArray:
> +        """
> +        Return a view on the array.
> +
> +        Returns
> +        -------
> +        ExtensionArray
> +
> +        Notes
>
> Do we have any restrictions on this being zero-copy?
>
> The test added in this PR will fail if we don't get an actual view.
>
> What are the consequences of .view returning self?
>
> view is going to be used by the default implementation of reshape, so
> returning self would cause all kinds of trouble.
>
> The default implementation should Just Work as long as self[:] returns a
> view, which should be the case anyway (JSONArray ATM returns a *copy*)
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27633?email_source=notifications&email_token=AAKAOIQSXEZTI25OC3NMILDQB46K5A5CNFSM4IHOGMHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB74RX5Y#discussion_r308392321>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOISH6L6AHLGM6ZNIXLLQB46K5ANCNFSM4IHOGMHA>
> .
>

> view is going to be used by the default implementation of reshape, so returning self would cause all kinds of trouble.

Can you be a bit more specific about this "all kings of trouble" ?
> Can you be a bit more specific about this "all kinds of trouble" ?

```
ser = pd.Series(some_ea)
df = ser.to_frame()
df.iloc[0, 0] = whatever
assert df.iloc[0] == ser.iloc[0]
```

would fail
Why would that fail if view would return self instead of a new object with a view on the same data?
> Why would that fail if view would return self instead of a new object with a view on the same data?

I could have been clearer: the default reshape is going to look like:

```
_shape = None

@property
def shape(self):
    if self._shape is not None:
         return self._shape
    return (self.size,)

def reshape(self, shape):
     out = self.view()
     out._shape = shape
     return out
```

So returning `self` instead of a new object would end up changing the shape of `self`.  That's a different failure mode from what would happen if `view` returned a copy, which I could have been clearer on.
test simplified, green
lgtm. ping on resolution of @jorisvandenbossche comments.
@jorisvandenbossche i think ive addressed your comments.  let me know if i missed anything
@jbrockmendel can you also answer to my comment / questions? (and not just follow it; I would like to have discussion about this)
> can you also answer to my comment / questions? (and not just follow it; I would like to have discussion about this)

The remaining question/comment I see is about DTA/TDA/PA view sometimes returning ndarray and the possibility of making it conform by always returning EA.  I like that idea eventually, but we're a ways away from that in terms of having PandasArray support throughout the codebase.  In a number of places where we currently do DTA.view, we expect to get an ndarray back.
> about DTA/TDA/PA view sometimes returning ndarray and the possibility of making it conform by always returning EA. I like that idea eventually, but we're a ways away from that in terms of having PandasArray support throughout the codebase. In a number of places where we currently do DTA.view, we expect to get an ndarray back.

But I don't think we *need* to wait on all-EA to have EA.view to consistently return an EA? For those cases where you expect an array, you can explicitly access the numpy array and take a view of that (or in the different order)? 

Eg one case where it is called is

https://github.com/pandas-dev/pandas/blob/9c37226a228784e993a220a02424fc8b68fe8257/pandas/core/arrays/datetimes.py#L222

We can use `self._data.view('i8')` or in this case `self.asi8` or other ways to ensure that the output is a ndarray
@jorisvandenbossche I'll try this locally and see what it would take to make make it EA-only.
Yah this causes 230 test failures.  I'm on board with the idea of fixing these, bout would like to do so in a separate PR(s) so as to keep momentum here.
> Yah this causes 230 test failures. 

It might be that they are all coming from a rather limited number of call sites (the number of failures does not always indicate the number of lines to change to fix it ;))

> I'm on board with the idea of fixing these, bout would like to do so in a separate PR(s) so as to keep momentum here.

That's fine for me (although I would prefer to see it done before the next release, as this is kind-of public API). Do you open an issue for it?

> It might be that they are all coming from a rather limited number of call sites

Skimming through the test output, all I can confidently state is that the number of relevant call sites is less than 230.
@jorisvandenbossche opened #27831 to change DTA.view signature.  Anything else?
Nope, all good!