EA: support basic 2D operations #27142

jbrockmendel · 2019-06-30T14:33:52Z

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

…ails

…rcompat

pandas/core/arrays/datetimelike.py

jreback

can u add a test that asserts shape is defined
and that len is also reasonable (if it’s overridden)

TomAugspurger · 2019-06-30T14:43:26Z

Remind me, why is this necessary? Is this for the reshape mixin? This will be a breaking change for EA authors.

jbrockmendel · 2019-06-30T15:04:22Z

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.

…rcompat

jbrockmendel · 2019-07-02T00:30:03Z

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

jreback · 2019-07-02T00:35:11Z

can you update the title of the PR to better reflect things

jreback · 2019-07-02T00:40:37Z

pandas/core/arrays/_reshaping.py

+from pandas._libs.lib import is_integer
+
+
+def implement_2d(cls):


what is the reasoning behind making this a decorator as opposed to just having base class method? it seems much simpler and more obvious.

The main pushback on this idea was the onus put on EA authors who only support 1D. ATM this decorator mainly just renames __len__ to size so that it can re-define shape in a 2D-compatible way. So asking authors to use the decorator instead of doing that re-definition themselves is kind of a wash.

But the step after this is to patch __getitem__, __setitem__, take, __iter__, all of which are easier to do with the decorator than by asking authors to do it themselves.

Is the plan still to have an "opt me out of this, I natively support 2D arrays"? If we go down the route of putting every Block.values inside an EA (with PandasArray for the current NumPy-backed Blocks), then we'll want that, right?

Oh, I see you're applying this to subclasses, rather than ExtensionArray itself. Motivation for that?

Motivation for that?

less magic than a metaclass (and my first attempt at using a metaclass failed)

Is the plan still to have an "opt me out of this, I natively support 2D arrays"?

Defined EA._allows_2d = False. Authors set that to True if they implement this themselves. This decorator should be updated to check that and be a no-op in that case.

Oh, right, because simply decorating

@implements_2d class ExtensionArray:

would work for subclasses right? It has to be a metaclass. I'll poke around at the meta class approach to see what's going on. May be too magical.

But if we do go with a

doc/source/whatsnew/v0.25.0.rst

…rcompat

TomAugspurger · 2019-07-02T21:30:37Z

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

jbrockmendel · 2019-07-03T02:59:37Z

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.

jorisvandenbossche · 2019-07-03T03:46:48Z

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).

jbrockmendel · 2019-07-03T17:39:09Z

I am a bit lost in the different PRs / POC you made.

Fair enough. Quick summary:

WIP/POC: Allow (N, 1) and (1,N) EAs #26914, POC: 2D support for 1D ExtensionArray subclasses #26954, POC: 2D EAs via composition #27015 were all proof-of-concept PRs, now closed.
EA: preliminary EA reshape ops #27153 was to implement view, T, and ravel, but I decided to roll that into this PR, so that is now closed.
DEPR: make Categorical.ravel() return Categorical #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".

…rcompat

TomAugspurger · 2019-07-23T17:01:27Z

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

jreback · 2019-07-23T17:08:58Z

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

…

On Jul 23, 2019, at 1:01 PM, Tom Augspurger ***@***.***> 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.

jbrockmendel · 2019-07-23T17:09:59Z

Today after 4 EST works for me.

TomAugspurger · 2019-07-23T18:17:26Z

Let's do 4:00 EST / 3:00 Central / 1:00 Pacific then. meet.google.com/bxs-pjaf-oqi

TomAugspurger

Still looking through the patched methods.

pandas/core/arrays/_reshaping.py

TomAugspurger · 2019-07-23T19:36:25Z

pandas/core/arrays/_reshaping.py

+
+    def __iter__(self):
+        if self.ndim == 1:
+            for obj in orig_iter(self):


Does just return orig_iter(self) work?

IIRC from one of the proofs-of-concept this actually mattered, but I dont remember which direction it mattered in

TomAugspurger · 2019-07-23T19:37:23Z

pandas/core/arrays/_reshaping.py

+            for obj in orig_iter(self):
+                yield obj
+        else:
+            for n in range(self.shape[0]):


Can this case be written as something like

reduced = self[:, 0] return orig_iter(reduced)`

? I worry a bit about the cost of calling __getitem__ on each iteration.

Definitely dont want to call orig_iter here since we want to yield ndim-1 arrays

TomAugspurger · 2019-07-23T19:41:07Z

pandas/core/arrays/_reshaping.py

+from pandas._libs.lib import is_integer
+
+
+def implement_2d(cls):


Is the plan still to have an "opt me out of this, I natively support 2D arrays"? If we go down the route of putting every Block.values inside an EA (with PandasArray for the current NumPy-backed Blocks), then we'll want that, right?

TomAugspurger · 2019-07-23T19:44:49Z

pandas/core/arrays/_reshaping.py

+
+        key = expand_key(key, self.shape)
+        if is_integer(key[0]):
+            assert key[0] in [0, -1]


I don't understand this at a glance. Who does it have to be in [0, -1]? (I get that we're only allowing N x 1 and 1 X N arrays, but I don't get why key[0] is the one being checked).

Just from looking at it, I would expect this to break this __getitem__ on something like

arr = pd.array([1, 2, 3], dtype="Int64") # arr = arra.reshape(...) arr[1]

I think I wrote this with only (1, N) in mind

TomAugspurger · 2019-07-23T19:46:42Z

pandas/core/arrays/_reshaping.py

+    """
+    if len(shape) == 1:
+        return True
+    if len(shape) > 2:


Does this come up in practice? I'm having a hard time judging whether NotImplementedError or False is the right behavior in this case.

I think NotImplementedError is more accurate, but for the current implementation this should never be reached

TomAugspurger · 2019-07-23T19:48:06Z

pandas/core/arrays/_reshaping.py

+def tuplify_shape(size: int, shape, restrict=True) -> Tuple[int, ...]:
+    """
+    Convert a passed shape into a valid tuple.
+    Following ndarray.reshape, we accept either `reshape(a, b)` or


I don't suppose NumPy has a function we can borrow here? I'm not aware of one.

numpy/numpy#13768 is the closest thing I found, but I agree it seems like the kind of thing that should exist.

Here's a somewhat dumb approach:

def tuplify_shape(size, shape): return np.broadcast_to(None, size).reshape(*shape).shape

Tested as:

In [20]: tuplify_shape(100, (2, -1, 5)) Out[20]: (2, 10, 5) In [21]: tuplify_shape(100, ((2, -1, 5))) Out[21]: (2, 10, 5) In [22]: tuplify_shape(100, ((2, 11, 5))) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) <ipython-input-22-f71a2f488ea8> in <module>() ----> 1 tuplify_shape(100, ((2, 11, 5))) <ipython-input-19-6b4139eccdff> in tuplify_shape(size, shape) 1 def tuplify_shape(size, shape): ----> 2 return np.broadcast_to(None, size).reshape(*shape).shape ValueError: cannot reshape array of size 100 into shape (2,11,5)

TomAugspurger · 2019-07-23T19:50:12Z

pandas/core/arrays/base.py

+        -----
+        - This must return a *new* object, not self.
+        - The only case that *must* be implemented is with dtype=None,
+          giving a view with the same dtype as self.


Can / should we make any requirements on this being no-copy? i.e.

a = my_array(...) b = a.view(dtype=int) b[0] = 10 assert a[0] == b[0]

definitely needs to be no-copy; i'll add that to the docstring

there is a test for the no-copy bit

TomAugspurger · 2019-07-23T19:50:54Z

pandas/core/arrays/base.py

@@ -932,6 +977,26 @@ def _formatting_values(self) -> np.ndarray:
    # Reshaping
    # ------------------------------------------------------------------------

+    def reshape(self, *shape):
+        """
+        Return a view on this array with the given shape.


Would be good to document that only (1, N) and (N, 1) is allowed here.

Good thinking. ATM I wrote most of this with only (1, N) in mind

TomAugspurger · 2019-07-23T19:51:44Z

pandas/core/arrays/_reshaping.py

+from pandas._libs.lib import is_integer
+
+
+def implement_2d(cls):


Oh, I see you're applying this to subclasses, rather than ExtensionArray itself. Motivation for that?

jreback · 2019-07-23T20:03:21Z

@TomAugspurger what is the google link?

jorisvandenbossche · 2019-07-28T15:03:39Z

Did you have a call? If so, is there a summary of what has been discussed?

jbrockmendel · 2019-07-29T00:53:25Z

@jorisvandenbossche yes we did, but nothing written AFAIK. The main takeaway I got is that jreback is on board contingent on there being zero additional (required) complexity for downstream authors.

…rcompat

TomAugspurger · 2019-07-29T15:24:28Z

Which essentially means a either

metaclass to patch take, getitem, etc. at import time.
monkeypatch or something at runtime when the array gets to internals.

My preference is for 1 if it's doable.

…rcompat

jreback · 2019-09-08T16:13:20Z

doc/source/whatsnew/v0.25.0.rst

@@ -888,6 +888,7 @@ Other API changes
 - :meth:`ExtensionArray.argsort` places NA values at the end of the sorted array. (:issue:`21801`)
 - :meth:`DataFrame.to_hdf` and :meth:`Series.to_hdf` will now raise a ``NotImplementedError`` when saving a :class:`MultiIndex` with extention data types for a ``fixed`` format. (:issue:`7775`)
 - Passing duplicate ``names`` in :meth:`read_csv` will now raise a ``ValueError`` (:issue:`17346`)
+- :meth:`Categorical.ravel` will now return a :class:`Categorical` instead of a NumPy array. (:issue:`27153`)


is this change in 0.25? (IIRC yes?), if not can you break it out

i think we only recently deprecated the old behavior. will double-check and see whats appropriate

jreback · 2019-09-08T16:16:12Z

pandas/core/arrays/_reshaping.py

+
+    cls.__iter__ = __iter__
+
+    return cls


Not sure if we are still doing this approach, but if we are

I would break out each of the implementations into a well named function that takes cls. then impelemented_2d is pretty straightforward to read w/o having to understand the details, you can immediately see what is being changed and the details exist in the functions.

Agreed.

ATM the relevant discussion about how to proceed is in Tom's PR here

jbrockmendel · 2019-09-16T17:14:45Z

Closing pending #28389.

jbrockmendel added 6 commits June 28, 2019 14:22

un-xfail tests, xfail instead of skip, minor cleanup

d673008

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

b2a837b

…ails

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

c2fd1b1

…ails

REF: derive __len__ from shape instead of vice-versa

bed5563

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

29d9dc2

…rcompat

update docstring

f3ce13c

jreback requested changes Jun 30, 2019

View reviewed changes

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

jreback requested changes Jun 30, 2019

View reviewed changes

remove duplicated methods

4fd24c1

jbrockmendel mentioned this pull request Jun 30, 2019

EA: preliminary EA reshape ops #27153

Closed

jbrockmendel added 5 commits July 1, 2019 14:34

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

c0505ee

…rcompat

implement shape in terms of size, with implement_2d decorator

c81daeb

move implement_2d, implement view

4d77dbe

port tests

d43ef30

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

91c979b

…rcompat

jreback requested changes Jul 2, 2019

View reviewed changes

jbrockmendel changed the title ~~EA: require EA to define shape instead of __len__~~ EA: support basic 2D operations Jul 2, 2019

TomAugspurger reviewed Jul 2, 2019

View reviewed changes

doc/source/whatsnew/v0.25.0.rst Show resolved Hide resolved

jbrockmendel added 2 commits July 2, 2019 06:20

shape patching, tests

bc220c3

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

421b5a3

…rcompat

jbrockmendel mentioned this pull request Jul 3, 2019

DEPR: make Categorical.ravel() return Categorical #27199

Merged

4 tasks

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

7c6df89

…rcompat

jbrockmendel added 7 commits July 22, 2019 08:48

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

81191bf

…rcompat

use base view

91639dd

patch take, getitem

34cc9e9

blackify

444f9f7

isort fixup

7c15b74

patch iter

768d75d

slice handling cleanup

3b7b2b2

TomAugspurger reviewed Jul 23, 2019

View reviewed changes

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

177bfb0

…rcompat

jbrockmendel mentioned this pull request Jul 29, 2019

EA: implement+test EA.view #27633

Merged

jbrockmendel added 5 commits August 1, 2019 17:21

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

f5cba22

…rcompat

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

174a1da

…rcompat

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

1d78fbe

…rcompat

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

41b49d9

…rcompat

dummy to force CI

fc331b8

jbrockmendel mentioned this pull request Sep 5, 2019

Metaclass approach jbrockmendel/pandas#3

Closed

jreback requested changes Sep 8, 2019

View reviewed changes

jbrockmendel mentioned this pull request Sep 11, 2019

EA: require size instead of __len__ #28389

Closed

jbrockmendel closed this Sep 16, 2019

jorisvandenbossche mentioned this pull request Jan 22, 2021

ENH: 2D support for MaskedArray #38992

Merged

jbrockmendel deleted the arrcompat branch November 20, 2021 23:23

		from pandas._libs.lib import is_integer


		def implement_2d(cls):

EA: support basic 2D operations #27142

EA: support basic 2D operations #27142

Conversation

jbrockmendel commented Jun 30, 2019

jreback left a comment

Choose a reason for hiding this comment

TomAugspurger commented Jun 30, 2019

jbrockmendel commented Jun 30, 2019

jbrockmendel commented Jul 2, 2019

jreback commented Jul 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Jul 2, 2019

jbrockmendel commented Jul 3, 2019

jorisvandenbossche commented Jul 3, 2019

jbrockmendel commented Jul 3, 2019

TomAugspurger commented Jul 23, 2019

jreback commented Jul 23, 2019 via email

jbrockmendel commented Jul 23, 2019

TomAugspurger commented Jul 23, 2019

TomAugspurger left a 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

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 Jul 23, 2019

jorisvandenbossche commented Jul 28, 2019

jbrockmendel commented Jul 29, 2019

TomAugspurger 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

Choose a reason for hiding this comment

jbrockmendel commented Sep 16, 2019