EA: implement+test EA.view #27633

jbrockmendel · 2019-07-29T02:44:22Z

Broken off from #27142, plus some type annotations

TomAugspurger · 2019-07-29T17:59:02Z

pandas/core/arrays/base.py

+        -------
+        ExtensionArray
+
+        Notes


I've been inconsistent about this, be in general I've tried to keep docstrings user-facing. For implementation notes I've just used regular comments.

What are the consequences of .view returning self?

Do we have any restrictions on this being zero-copy?

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)

Convert this to normal comments as Tom mentioned?

TomAugspurger · 2019-07-29T19:16:06Z

SGTM.

…

On Mon, Jul 29, 2019 at 2:12 PM jbrockmendel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/core/arrays/base.py <#27633 (comment)>: > @@ -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 <#27633?email_source=notifications&email_token=AAKAOIQSXEZTI25OC3NMILDQB46K5A5CNFSM4IHOGMHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB74RX5Y#discussion_r308392321>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOISH6L6AHLGM6ZNIXLLQB46K5ANCNFSM4IHOGMHA> .

jorisvandenbossche · 2019-07-30T08:27:29Z

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" ?

jreback · 2019-07-30T12:01:27Z

pandas/core/arrays/categorical.py

@@ -1773,9 +1766,10 @@ def view(self):
        Returns
        -------
        view : Categorical
-           Returns `self`!


can you update the doc-string (or just inherit it)

see my comment above, dtype needs to be added (but best to just remove the doc-string completely and inherit it)

pandas/core/arrays/datetimelike.py

jreback · 2019-07-30T12:02:18Z

pandas/tests/extension/base/interface.py

+
+    def test_view(self, data):
+        # view with no dtype should return a shallow copy, *not* the same
+        #  object


do you need to also test with a dtype != None? (e.g. that this raises NIE)

I guess to make sure the kwarg is accepted, sure

pandas/tests/extension/test_interval.py

pandas/core/arrays/base.py

jbrockmendel · 2019-07-30T14:50:12Z

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

jorisvandenbossche · 2019-07-30T15:01:42Z

Why would that fail if view would return self instead of a new object with a view on the same data?

jbrockmendel · 2019-07-30T15:56:15Z

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.

pandas/core/arrays/base.py

jreback · 2019-07-31T12:23:30Z

pandas/core/arrays/categorical.py

@@ -1773,9 +1766,10 @@ def view(self):
        Returns
        -------
        view : Categorical
-           Returns `self`!


see my comment above, dtype needs to be added (but best to just remove the doc-string completely and inherit it)

pandas/core/arrays/datetimelike.py

pandas/tests/extension/base/interface.py

jreback

lgtm ex the comment on the test. ping on green.

jbrockmendel · 2019-08-02T15:20:02Z

test simplified, green

jorisvandenbossche · 2019-08-02T16:10:49Z

pandas/core/arrays/base.py

@@ -354,7 +355,7 @@ def ndim(self) -> int:
        """
        Extension Arrays are only allowed to be 1-dimensional.
        """
-        return 1
+        return len(self.shape)


I am not sure this change will help. There can be EAs out there that define this themselves and override this with a hardcoded 1, so we will still need to define a wrapper I think?
(so therefore I would maybe rather leave it as is, to ensure we cover that use case)

im not sure I understand the problem here. is there a case in which this wont be correct?

I supposed you made this change because for the other PR you are patching self.shape to return (N, 1) or (1, N), and so with this change, ndim automatically follows that.
But in general, you can't rely on the fact that self.ndim already is correctly following self.shape, so you will always have to patch ndim as well.

(exact terminology of "patching" might not be fully reflect to other PR on 2D EAs, need to update myself on that, but hope to give the idea)

will revert so we'll handle it in the next pass

jorisvandenbossche · 2019-08-02T16:12:17Z

pandas/core/arrays/base.py

+        -------
+        ExtensionArray
+
+        Notes


Convert this to normal comments as Tom mentioned?

pandas/core/arrays/base.py

jorisvandenbossche · 2019-08-02T16:16:05Z

pandas/core/arrays/categorical.py

+    def view(self, dtype=None):
+        if dtype is not None:
+            raise NotImplementedError(dtype)
+        return self._constructor(values=self._codes, dtype=self.dtype, fastpath=True)


the default implementation does not work here? (or is this more efficient?)

This is more efficient, yes (note the fastpath kwarg)

Can you add a note for that? (eg "override base implementation to use fastpath")

jorisvandenbossche · 2019-08-02T16:16:44Z

pandas/core/arrays/datetimelike.py

-            With the specified `dtype`.
-        """
+        if dtype is None or dtype is self.dtype:
+            return type(self)(self._data, dtype=self.dtype)
        return self._data.view(dtype=dtype)


This is not returning an EA?

correct, the current implementation is only used to return an ndarray.

But so that is "violating" the spec? (it should return a new EA (not self), but not an ndarray)

yes, but its already in place and used extensively. I guess we could alter the spec to allow returning ndarray

I guess we could alter the spec to allow returning ndarray

Sorry to further bother on this PR, but I would not alter the spec. For the interface, it should just be a new EA of the same type, no?

Shouldn't we (ideally, at some point) change our own implementation to return an EA as well for consistency?

jreback · 2019-08-02T16:23:34Z

lgtm. ping on resolution of @jorisvandenbossche comments.

jbrockmendel · 2019-08-02T21:39:33Z

@jorisvandenbossche i think ive addressed your comments. let me know if i missed anything

jorisvandenbossche · 2019-08-03T19:04:31Z

pandas/core/arrays/base.py

+
+        Returns
+        -------
+        ExtensionArray or np.ndarray


I would not document this. For implementors, it is simply an EA that it should be. I think our own array not doing can be seen as an historical artifact that ideally will be fixed?

jorisvandenbossche · 2019-08-03T19:07:07Z

pandas/core/arrays/datetimelike.py

-            With the specified `dtype`.
-        """
+        if dtype is None or dtype is self.dtype:
+            return type(self)(self._data, dtype=self.dtype)
        return self._data.view(dtype=dtype)


I guess we could alter the spec to allow returning ndarray

Sorry to further bother on this PR, but I would not alter the spec. For the interface, it should just be a new EA of the same type, no?

Shouldn't we (ideally, at some point) change our own implementation to return an EA as well for consistency?

jorisvandenbossche · 2019-08-04T08:04:24Z

@jbrockmendel can you also answer to my comment / questions? (and not just follow it; I would like to have discussion about this)

jbrockmendel · 2019-08-04T15:42:27Z

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.

jreback · 2019-08-04T22:18:17Z

pandas/core/arrays/base.py

@@ -862,6 +863,27 @@ def copy(self) -> ABCExtensionArray:
        """
        raise AbstractMethodError(self)

+    def view(self, dtype=None) -> Union[ABCExtensionArray, np.ndarray]:


why would this return a np.ndarray OR an EA for an EA? (is this @jorisvandenbossche question)?

when / why would this be the case? this is pretty confusing.

Yes, this is the same as Joris's question. The answer is that it is probably better to return EA-only, but ATM DTA/TDA/PA have existing implementations that return ndarray

jorisvandenbossche · 2019-08-05T06:40:02Z

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

pandas/pandas/core/arrays/datetimes.py

Line 222 in 9c37226

result = op(self.view("i8"), other.view("i8"))

We can use self._data.view('i8') or in this case self.asi8 or other ways to ensure that the output is a ndarray

jbrockmendel · 2019-08-05T17:07:37Z

@jorisvandenbossche I'll try this locally and see what it would take to make make it EA-only.

jbrockmendel · 2019-08-05T18:19:47Z

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.

jorisvandenbossche · 2019-08-06T20:19:03Z

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?

jbrockmendel · 2019-08-06T21:31:39Z

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.

jbrockmendel · 2019-08-09T02:56:39Z

@jorisvandenbossche opened #27831 to change DTA.view signature. Anything else?

jorisvandenbossche · 2019-08-09T08:00:35Z

Nope, all good!

jbrockmendel added 2 commits July 28, 2019 19:43

implement+test EA.view

c368fb8

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

27a0af2

TomAugspurger reviewed Jul 29, 2019

View reviewed changes

jreback requested changes Jul 30, 2019

View reviewed changes

jreback added ExtensionArray Extending pandas with custom dtypes or arrays. API Design labels Jul 30, 2019

jreback reviewed Jul 30, 2019

View reviewed changes

pandas/core/arrays/base.py Outdated Show resolved Hide resolved

jbrockmendel added 3 commits July 30, 2019 09:57

test, comment

6636c89

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

a311f6e

fix broken case

f99afb5

jreback added this to the 1.0 milestone Jul 31, 2019

jreback requested changes Jul 31, 2019

View reviewed changes

jbrockmendel added 6 commits July 31, 2019 15:26

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

1c3d81c

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

cddcab4

docstring edits

aa290bf

add view to doc

6053a50

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

426c434

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

67090a6

jreback requested changes Aug 2, 2019

View reviewed changes

jbrockmendel added 2 commits August 2, 2019 07:35

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

d84a0ba

simplify test

d9158ef

jorisvandenbossche reviewed Aug 2, 2019

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into ac1

86c9ab9

jbrockmendel added 2 commits August 2, 2019 09:46

docstring/comment edits

2b5e5fa

requested edits

24b235c

jorisvandenbossche reviewed Aug 3, 2019

View reviewed changes

revert return type

c44aada

jreback requested changes Aug 4, 2019

View reviewed changes

jbrockmendel mentioned this pull request Aug 9, 2019

EA: DatetimelikeArray.view should always return EA #27831

Closed

jorisvandenbossche merged commit 0227e69 into pandas-dev:master Aug 9, 2019

jbrockmendel deleted the ac1 branch August 9, 2019 14:34

quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request Aug 16, 2019

EA: implement+test EA.view (pandas-dev#27633)

508ec0d

EA: implement+test EA.view #27633

EA: implement+test EA.view #27633

Conversation

jbrockmendel commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Jul 29, 2019 via email

jorisvandenbossche commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jul 30, 2019

jorisvandenbossche commented Jul 30, 2019

jbrockmendel commented Jul 30, 2019

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jbrockmendel commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Aug 2, 2019

jbrockmendel commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Aug 4, 2019

jbrockmendel commented Aug 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Aug 5, 2019

jbrockmendel commented Aug 5, 2019

jbrockmendel commented Aug 5, 2019

jorisvandenbossche commented Aug 6, 2019

jbrockmendel commented Aug 6, 2019

jbrockmendel commented Aug 9, 2019

jorisvandenbossche commented Aug 9, 2019

jorisvandenbossche Aug 2, 2019 •

edited

Loading