API: dispatch to EA.astype #22343

TomAugspurger · 2018-08-14T14:32:39Z

split from #22325

Note that this is just a partial fix to get sparse working. We have some issues:

for better or worse, categorical is still special cased. Series[EA].astype('category') will not yet dispatch to EA.astype. That's good non-pandas EAs won't have to worry about specially handling categorical. It's bad, since it's a special case.

And that's the second issue, in general, how should we handle astyping from EA1 -> EA2? In principle, we always have ndarray[object] as the lowest common denominator. So something like EA2(EA.astype(object)) may work. But that won't necessarily be efficient. I'm not sure what's best here.

cc @xhochy

Closes pandas-dev#21185

TomAugspurger · 2018-08-14T14:35:01Z

pandas/core/arrays/integer.py

@@ -391,7 +392,7 @@ def astype(self, dtype, copy=True):

        # coerce
        data = self._coerce_to_ndarray()
-        return data.astype(dtype=dtype, copy=False)
+        return astype_nansafe(data, dtype, copy=None)


This was necessary so that

In [8]: s.values Out[8]: IntegerArray([0, nan, 2], dtype='Int8') In [9]: s.astype('uint32')

still raises. Previously, it did

In [4]: pd.core.arrays.IntegerArray([1, None, 2], dtype='uint8').astype('uint32') Out[4]: array([ 1, 2415919104, 2], dtype=uint32)

which I don't think we want. This was only tested at the Series[IntegerArray].astype level, which never called EA.astype

Can you add a test for this?

TomAugspurger · 2018-08-14T14:37:43Z

pandas/tests/extension/decimal/array.py

@@ -15,6 +15,17 @@ class DecimalDtype(ExtensionDtype):
    name = 'decimal'
    na_value = decimal.Decimal('NaN')

+    def __init__(self, context=None):


Seemed like a good idea to have a parameterized ExtensionDtype in the tests folder. For .astype, we're testing that Series[decimal].astype(DecimalDtype(new_context)) is passed all the way through.

TomAugspurger · 2018-08-14T14:38:48Z

pandas/tests/extension/decimal/array.py

+    def __init__(self, context=None):
+        self.context = context or decimal.getcontext()
+
+    def __eq__(self, other):


Our default eq is dangerous for parametrized dtypes, depending on whether the parameters appear in the name :/

codecov · 2018-08-14T16:16:46Z

Codecov Report

Merging #22343 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22343      +/-   ##
==========================================
+ Coverage   92.05%   92.05%   +<.01%     
==========================================
  Files         169      169              
  Lines       50709    50712       +3     
==========================================
+ Hits        46679    46682       +3     
  Misses       4030     4030

Flag	Coverage Δ
#multiple	`90.46% <100%> (ø)`	⬆️
#single	`42.25% <72.72%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/internals/blocks.py	`93.84% <100%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`88.58% <100%> (ø)`	⬆️
pandas/core/arrays/integer.py	`94.69% <100%> (+0.02%)`	⬆️
pandas/core/frame.py	`97.24% <0%> (ø)`	⬆️
pandas/core/series.py	`93.72% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf70d11...2606d02. Read the comment docs.

jorisvandenbossche · 2018-08-14T18:37:11Z

pandas/tests/extension/decimal/test_decimal.py

@@ -205,6 +205,24 @@ def test_dataframe_constructor_with_dtype():
    tm.assert_frame_equal(result, expected)


+@pytest.mark.parametrize("frame", [True, False])
+def test_astype_dispatches(frame):
+    data = pd.Series(DecimalArray([decimal.Decimal(2)]), name='a')


can you add a comment here why this test is added here (instead of as generic extension test)

jorisvandenbossche · 2018-08-14T18:44:48Z

Code changes look good!

Regarding the discussion items you mention, I am wondering if we need some kind of "negotation protocol" between (EA) dtypes for this. Basically something like:

consider Series(EA).astype(dtype)
first try EA.astype(dtype) -> if the EA knows how to handle the specific dtype, return a new EA/array; if it does not know how to convert itself to dtype, return NotImplemented
if the above returned NotImplemented, ask dtype if it knows how to convert the EA values to its own dtype -> if it knows, return a new EA/array; otherwise return NotImplemented
if both don't know (above return NotImplemented again), either raise an error or fall back to the base implementation (np.array(self, dtype=dtype,, which might also raise an error)

That would in principle enable EA.astype(EA2), and might also mitigate the 'category' problem you mention.

TomAugspurger · 2018-08-14T20:24:11Z

Your negotiation seems sensible I think. I worry a bit about a divergence between Series.astype(dtype) and Series.values.astype(dtype), but that's maybe unavoidable.

edit: though it would be strange (wrong) for a method like ExtensionArray.astype to return NotImplemented, so this would have to be a back-channel like ExtensionArray._astype, which is the same as astype, but can return NotImplemented? This seems messy.

jorisvandenbossche · 2018-08-14T21:57:15Z

I worry a bit about a divergence between Series.astype(dtype) and Series.values.astype(dtype)

That's a good point, yeah, that would be strange. Such a negotiation does not really fit for a method, only for functions probably ..

h-vetinari · 2018-08-15T07:24:14Z

Such a negotiation does not really fit for a method, only for functions probably...

How about raising (and catching) an UnsupportedConversionError (but letting other errors pass through)? That should easily be compatible with method calls.

TomAugspurger · 2018-08-15T11:48:14Z

Perhaps. We'll leave that for another issue though I think, if we discover a need for it in practice.

jreback · 2018-08-15T11:55:02Z

pandas/core/internals/blocks.py

@@ -637,22 +637,25 @@ def _astype(self, dtype, copy=False, errors='raise', values=None,
            # force the copy here
            if values is None:

-                if issubclass(dtype.type,
-                              (compat.text_type, compat.string_types)):
+                if self.is_extension:


@TomAugspurger are you really really sure this is needed?
all of this already works with IntegerArray so not t really sure what problem you are trying to solve

Needed for #22325.

Sparse has special semantics for .astypeing, Series[sparse].astype(np.dtype) is interpreted as astyping the underlying .values.sp_values, rather than densifying and asytping. I'd like to deprecate this, but that's another matter.

In general though, it seems like EAs should have a say in how they're astyped, rather than always going through .get_values.

we already do this is my point

Not on master? That hit's https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals/blocks.py#L652, which converts to an ndarray, before ever calling the extension array's astype.

At least, my new tests fail on master without these changes. I'm not sure if / how IntegerArray is being handled differently.

do you mean the current code in master or this PR?

this PR

there is a ton of code in astype to dispatch to extension types already
this adds yet another branch

Can you point to this "ton of code"? I don't see any other dispatch to EAs.
What is done is the generic conversion to array and then the use of astype_nansafe (which has a dispatch to EAs, but for the target EA dtype, not for the calling EA dtype)

It's a 2 line change, this extra if condition. Which of those two lines is the convoluted one?

hmm, I could swear I add an .is_extension branch already, that's why this looks weird. In any event this has lots of if/then condition. Please make a note / issue to clean this up.

jreback · 2018-08-15T13:36:13Z

pandas/core/dtypes/cast.py

+        try:
+            return arr.view(dtype)
+        except TypeError:
+            if copy is None:


why is this necessary? what breaks this?

There was some object astype that failed (not sure if it's to or from object). I've reverted this change, we'll see what fails.

In [10]: np.array([1, 2, 3], dtype='int64').view(object) ... TypeError: Cannot change data-type for object array.

Ah, yes. thanks. Pushed a simpler fix that hopefully works.

jreback · 2018-08-15T13:36:28Z

pandas/core/dtypes/cast.py

+            if copy is None:
+                # allowed to copy if necessary (e.g. object)
+                return arr.astype(dtype, copy=True)
+            else:


don't need an else

jreback · 2018-08-15T13:39:41Z

pandas/core/internals/blocks.py

@@ -637,22 +637,25 @@ def _astype(self, dtype, copy=False, errors='raise', values=None,
            # force the copy here
            if values is None:

-                if issubclass(dtype.type,
-                              (compat.text_type, compat.string_types)):
+                if self.is_extension:


hmm, I could swear I add an .is_extension branch already, that's why this looks weird. In any event this has lots of if/then condition. Please make a note / issue to clean this up.

jorisvandenbossche · 2018-08-15T14:01:55Z

pandas/core/dtypes/cast.py

-    copy : bool or None, default True
-        Whether to copy during the `.astype` (True) or
-        just return a view (False). Passing `copy=None` will
-        attempt to return a view, but will copy if necessary.


the copy keyword still exists, so I would not remove it completely

jreback

small test comment. is the references closing issue at the top-of-the PR correct? (it is the original EA dtype registry)?

jreback · 2018-08-16T10:50:09Z

pandas/tests/extension/decimal/test_decimal.py

@@ -205,6 +205,27 @@ def test_dataframe_constructor_with_dtype():
    tm.assert_frame_equal(result, expected)


+@pytest.mark.parametrize("frame", [True, False])


why don't we add a test in base which raises NotImplementedError so that authors are forced to cover this?

But is it useful to force EA authors to do it? This is basically checking the pandas dispatch (which we can do here), not the actual EA.astype implementation

Or how about something like

@pytest.mark.parametrize('typ, check', [ ('category', 'is_categorical_dtype'), ... ]) def test_astype_category(self, data): assert check(data.astype(dtype))

Would that make sense as a base test? I think our default implementation would need to be updated to not fail that.

My main concern is that it would be difficult to override (e.g. skip) just some of the dtypes, so maybe we would have to write those as separate tests?

I'm mildly concerned that EA authors will not correctly handle extension types. Our base implementation currently fails to handle them. Although we document it as Cast to a NumPy array with 'dtype'.. Should we make the return type Union[ndarray, ExtensionArray]?

I would leave such a test until we better figure out how to handle those cross EA/non-EA astype calls (the discussion we were having above in this PR )

Should we make the return type Union[ndarray, ExtensionArray]?

Yeah, probably yes in any case (since EAs authors that provide multiple dtypes already do that, like IntegerArray)

I would add an issue to make this test, not blocking this PR on it.

TomAugspurger · 2018-08-16T11:18:27Z

Fixed the referenced issue.

jorisvandenbossche · 2018-08-16T11:21:08Z

That's the same issue as for #22345?

jorisvandenbossche · 2018-08-16T11:23:20Z

pandas/core/dtypes/cast.py

+    ----------
+    arr : ndarray
+    dtype : np.dtype
+    copy : bool, default True


Can you specify here what happens with False? (will always do view (except for object), even if the itemsize is incompatible)

jorisvandenbossche · 2018-08-16T11:25:18Z

pandas/tests/extension/decimal/test_decimal.py

@@ -205,6 +205,27 @@ def test_dataframe_constructor_with_dtype():
    tm.assert_frame_equal(result, expected)


+@pytest.mark.parametrize("frame", [True, False])


But is it useful to force EA authors to do it? This is basically checking the pandas dispatch (which we can do here), not the actual EA.astype implementation

jorisvandenbossche · 2018-08-16T11:26:07Z

pandas/tests/extension/integer/test_integer.py

+
+        with tm.assert_raises_regex(
+                ValueError, 'cannot convert float NaN to integer'):
+            arr.astype('uint32')


Can you put this as a stand-alone test at the bottom of this file? (it's not overwriting a base test, but it is a custom integer-specific test)

TomAugspurger · 2018-08-16T11:26:37Z

Ugh, too early. #21296 is the one.

TomAugspurger · 2018-08-20T11:05:15Z

@jreback was your comment about the wrong issue number all that was left from your point of view? If so, this should be good to go.

jreback · 2018-08-20T11:08:24Z

thanks @TomAugspurger

maybe need to modify text in pandas/arrays/base/ExtensionArray (the doc-string) to add astype)?

API: dispatch to EA.astype

a7ba8f6

Closes pandas-dev#21185

TomAugspurger added Dtype Conversions Unexpected or buggy dtype conversions ExtensionArray Extending pandas with custom dtypes or arrays. labels Aug 14, 2018

TomAugspurger commented Aug 14, 2018

View reviewed changes

TomAugspurger added 2 commits August 14, 2018 11:15

Merge remote-tracking branch 'upstream/master' into ea-astype-dispatch

26993fe

py2 compat

6eeec11

jorisvandenbossche reviewed Aug 14, 2018

View reviewed changes

TomAugspurger added 2 commits August 14, 2018 15:16

explainers

f1b860f

linting

5c44275

lint

de1fb5b

jreback reviewed Aug 15, 2018

View reviewed changes

jreback requested changes Aug 15, 2018

View reviewed changes

try removing

f147635

TomAugspurger mentioned this pull request Aug 15, 2018

CLN: Simplify Block._astype #22369

Closed

jorisvandenbossche reviewed Aug 15, 2018

View reviewed changes

TomAugspurger added 2 commits August 15, 2018 09:02

Simpler than catching

767e3ee

Docstring

5602330

jorisvandenbossche mentioned this pull request Aug 16, 2018

WIP: EA SetArray #22382

Closed

jreback requested changes Aug 16, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Aug 16, 2018

jorisvandenbossche reviewed Aug 16, 2018

View reviewed changes

TomAugspurger mentioned this pull request Aug 16, 2018

API: astype mechanism for extension arrays #22384

Open

move test

2606d02

jorisvandenbossche approved these changes Aug 16, 2018

View reviewed changes

jreback approved these changes Aug 20, 2018

View reviewed changes

jreback merged commit a3c50a6 into pandas-dev:master Aug 20, 2018

TomAugspurger deleted the ea-astype-dispatch branch August 20, 2018 12:12

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

API: dispatch to EA.astype (pandas-dev#22343)

5cbc85b

		@@ -205,6 +205,27 @@ def test_dataframe_constructor_with_dtype():
		tm.assert_frame_equal(result, expected)


		@pytest.mark.parametrize("frame", [True, False])

API: dispatch to EA.astype #22343

API: dispatch to EA.astype #22343

Conversation

TomAugspurger commented Aug 14, 2018 • 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

codecov bot commented Aug 14, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jorisvandenbossche commented Aug 14, 2018

TomAugspurger commented Aug 14, 2018 • edited Loading

jorisvandenbossche commented Aug 14, 2018

h-vetinari commented Aug 15, 2018

TomAugspurger commented Aug 15, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

TomAugspurger Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Aug 16, 2018

jorisvandenbossche commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Aug 16, 2018

TomAugspurger commented Aug 20, 2018

jreback commented Aug 20, 2018

TomAugspurger commented Aug 14, 2018 •

edited

Loading

codecov bot commented Aug 14, 2018 •

edited

Loading

TomAugspurger commented Aug 14, 2018 •

edited

Loading

TomAugspurger Aug 16, 2018 •

edited

Loading

TomAugspurger Aug 16, 2018 •

edited

Loading