BUG: use EA.astype in ExtensionBlock.to_native_types #28841

jorisvandenbossche · 2019-10-08T13:56:19Z

This implements a ExtensionBlock.to_native_types to override the base class which converts to a numpy array.

To test this is a bit tricky. The current JSONArray we have in the extension tests does not have this issue. I would need to implement and add tests for a new ListArray to check this (I can ensure that this fixes the geopandas issue)

jorisvandenbossche · 2019-10-08T14:05:02Z

It is also annoying that this does not work for eg SparseArray, so currently I am falling back to the base implementation in case of any error ...

pandas/core/internals/blocks.py

jbrockmendel · 2019-10-08T16:54:05Z

pandas/core/internals/blocks.py

+    def to_native_types(self, slicer=None, na_rep="nan", quoting=None, **kwargs):
+        """override to use ExtensionArray astype for the conversion"""
+        try:
+            values = self.values


A bunch of this can be build outside of the try/except right?

jbrockmendel · 2019-10-08T16:56:44Z

to_native_types has always been a weird name to me. in practice it seems to be used for preparing __repr__ output, but elsewhere "native" refers to something like C types. Changing the naming scheme is definitely outside the scope of this PR, but maybe a comment or docstring indicating what this is doing?

Also, IIRC there is another similar piece of code in this file dealing with SparseArray.__setitem__ being disabled. Can any of that be shared?

jorisvandenbossche · 2019-10-09T07:46:56Z

Also, IIRC there is another similar piece of code in this file dealing with SparseArray.setitem being disabled. Can any of that be shared?

There is indeed a if is_sparse(self.values): and a similar catching of an error in where, but since the code is for the test completely unrelated, I don't see any way to shared that.

to_native_types has always been a weird name to me.

Completely agree. We actually also have a public Index.to_native_types, that we should probably just deprecate.

jorisvandenbossche · 2019-10-09T07:56:24Z

I get a mypy error: pandas/core/internals/blocks.py:2274: error: Definition of "to_native_types" in base class "ExtensionBlock" is incompatible with definition in base class "DatetimeBlock"

which is very strange, DatetimeBlock is not at all a base class of ExtensionBlock. The only relationship is that they both derive from the base Block class ..

cc @WillAyd @simonjayhawkins any idea how I can get this error go away?
So we currently already have multiple incompatible signatures. Eg DatetimeBlock.to_native_types and FloatBlock.to_native_types both have a different signature compared to Block.to_native_types. But now adding a ExtensionBlock.to_native_types (with the same signature as base Block.to_native_types) triggers this error.

jorisvandenbossche · 2019-10-09T08:07:08Z

Ah, I understand now. I didn't look at the location of the error. It is reporting that about DatetimeTZBlock, which indeed subclasses both DatetimeBlock and ExtensionBlock

jorisvandenbossche · 2019-10-09T08:11:33Z

Solved it (I think) by also overriding it in DatetimeTZBlock, which I think we needed to do anyway to pick DatetimeBlock's version (although the fact that no tests were failing might be a sign that the specialized DatetimeBlock.to_native_types is either not necessary or not properly tested)

pandas/core/internals/blocks.py

jorisvandenbossche · 2019-10-11T13:22:38Z

don't we already do something like this in the repr of EAs? we should have a same / generic mechanism, rather than implementing logic here.

I fully agree it would be nice to make that better, but right now, formatting on the one hand and csv on the other (which uses to_native_types) are two separate things, and fixing that seems out of scope for this PR.
For now, the only thing I am doing in this PR is actually using the EA's version of astype instead of converting to a ndarray first.

jreback · 2019-10-11T14:09:41Z

don't we already do something like this in the repr of EAs? we should have a same / generic mechanism, rather than implementing logic here.

I fully agree it would be nice to make that better, but right now, formatting on the one hand and csv on the other (which uses to_native_types) are two separate things, and fixing that seems out of scope for this PR.
For now, the only thing I am doing in this PR is actually using the EA's version of astype instead of converting to a ndarray first.

ok fair enough. do we have an issue for this distinction?

pandas/core/internals/blocks.py

jorisvandenbossche · 2019-10-11T14:23:49Z

The main remaining question is whether we want tests for this (or if we are happy with it being indirectly tested on geopandas). Certainly, ideally we test it here as well. But, we currently don't have any EA that has that problem.

But will check if I can do a minimal ListArray EA (without implementing it fully) to reproduce this issue.

jorisvandenbossche · 2019-10-11T15:55:40Z

OK, added a minimal ListArray and a test based on that just for this issue. And I checked that the test actually fails without the overriden to_native_types

jbrockmendel · 2019-10-11T16:04:39Z

pandas/tests/extension/list/array.py

@@ -0,0 +1,131 @@
+"""Test extension array for storing nested data in a pandas container.


I think newline after """

jbrockmendel · 2019-10-11T16:04:52Z

pandas/tests/extension/list/array.py

+
+    @classmethod
+    def construct_array_type(cls):
+        """Return the array type associated with this dtype


newline, period

jreback

LIstArray tests look good

pandas/core/internals/blocks.py

jreback · 2019-10-12T17:22:17Z

pandas/tests/extension/list/array.py

+            if copy:
+                return self.copy()
+            return self
+        elif pd.api.types.is_string_dtype(dtype) and not pd.api.types.is_object_dtype(


could import at the top

jorisvandenbossche · 2019-10-14T22:44:10Z

Any opinions about including this in 0.25.2 ?

Normally it would not meet my requirements to be included in a bug fix release, but in this case it solves a regression in geopandas (due to moving to ExtensionArray, and it's not something I can fix in geopandas). But not fully how risky this change is for other EAs.

cc @TomAugspurger

jreback · 2019-10-14T22:48:43Z

seems ok for 0.25.2

TomAugspurger · 2019-10-14T22:55:08Z

Fine to include in 0.25, but you should do it soon :)

…

On Oct 14, 2019, at 17:48, Jeff Reback ***@***.***> wrote: seems ok for 0.25.2 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

jorisvandenbossche · 2019-10-14T22:57:16Z

OK, then I will merge ;)

jorisvandenbossche · 2019-10-15T02:33:24Z

@meeseeksdev backport 0.25.x

lumberbot-app · 2019-10-15T02:33:53Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout 0.25.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 8c5941cd57e260f4c3552769e79d8d6dbd6283d9

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #28841: BUG: use EA.astype in ExtensionBlock.to_native_types'

Push to a named branch :

git push YOURFORK 0.25.x:auto-backport-of-pr-28841-on-0.25.x

Create a PR against branch 0.25.x, I would have named this PR:

"Backport PR #28841 on branch 0.25.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

jorisvandenbossche · 2019-10-15T02:38:40Z

Manual backport: #28985

BUG: use EA.astype in ExtensionBlock.to_native_types

b50c581

jorisvandenbossche added Bug ExtensionArray Extending pandas with custom dtypes or arrays. labels Oct 8, 2019

jorisvandenbossche added 2 commits October 8, 2019 16:02

fallback to conversion to numpy array (eg for sparse array)

890fe82

use super implementation for fallback

ded3d00

jorisvandenbossche mentioned this pull request Oct 8, 2019

Cannot write csv in 0.6.0 if geometry contains multipolygon geopandas/geopandas#1145

Closed

Merge remote-tracking branch 'upstream/master' into EA-native-types

330f455

jbrockmendel reviewed Oct 8, 2019

View reviewed changes

pandas/core/internals/blocks.py Show resolved Hide resolved

jbrockmendel reviewed Oct 8, 2019

View reviewed changes

override in DatetimeTZBlock as well

ef65a96

jbrockmendel reviewed Oct 10, 2019

View reviewed changes

pandas/core/internals/blocks.py Outdated Show resolved Hide resolved

jreback requested changes Oct 11, 2019

View reviewed changes

pandas/core/internals/blocks.py Show resolved Hide resolved

clean-up

ba55396

jreback added this to the 1.0 milestone Oct 11, 2019

jreback reviewed Oct 11, 2019

View reviewed changes

pandas/core/internals/blocks.py Show resolved Hide resolved

add minimal ListArray for test

1663b3d

jbrockmendel reviewed Oct 11, 2019

View reviewed changes

jorisvandenbossche added 2 commits October 12, 2019 14:19

docstring fixes

a5ba129

fix test to avoid deprecation warning

61eaf92

jreback reviewed Oct 12, 2019

View reviewed changes

jreback approved these changes Oct 12, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into EA-native-types

f517e1a

jorisvandenbossche modified the milestones: 1.0, 0.25.2 Oct 14, 2019

add whatsnew

10defeb

jorisvandenbossche merged commit 8c5941c into pandas-dev:master Oct 14, 2019

jorisvandenbossche deleted the EA-native-types branch October 14, 2019 23:33

lumberbot-app bot added the Still Needs Manual Backport label Oct 15, 2019

jorisvandenbossche added a commit to jorisvandenbossche/pandas that referenced this pull request Oct 15, 2019

BUG: use EA.astype in ExtensionBlock.to_native_types (pandas-dev#28841)

c9b2b65

jorisvandenbossche added a commit that referenced this pull request Oct 15, 2019

BUG: use EA.astype in ExtensionBlock.to_native_types (#28841) (#28985)

505b1a7

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG: use EA.astype in ExtensionBlock.to_native_types (pandas-dev#28841)

4332415

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG: use EA.astype in ExtensionBlock.to_native_types (pandas-dev#28841)

0e9865f

bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020

BUG: use EA.astype in ExtensionBlock.to_native_types (pandas-dev#28841)

1d64388

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: use EA.astype in ExtensionBlock.to_native_types #28841

BUG: use EA.astype in ExtensionBlock.to_native_types #28841

jorisvandenbossche commented Oct 8, 2019

jorisvandenbossche commented Oct 8, 2019

jbrockmendel Oct 8, 2019

jbrockmendel commented Oct 8, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 11, 2019

jreback commented Oct 11, 2019

jorisvandenbossche commented Oct 11, 2019

jorisvandenbossche commented Oct 11, 2019

jbrockmendel Oct 11, 2019

jbrockmendel Oct 11, 2019

jreback left a comment

jreback Oct 12, 2019

jorisvandenbossche commented Oct 14, 2019

jreback commented Oct 14, 2019

TomAugspurger commented Oct 14, 2019 via email

jorisvandenbossche commented Oct 14, 2019

jorisvandenbossche commented Oct 15, 2019

lumberbot-app bot commented Oct 15, 2019

jorisvandenbossche commented Oct 15, 2019

		@@ -0,0 +1,131 @@
		"""Test extension array for storing nested data in a pandas container.

BUG: use EA.astype in ExtensionBlock.to_native_types #28841

BUG: use EA.astype in ExtensionBlock.to_native_types #28841

Conversation

jorisvandenbossche commented Oct 8, 2019

jorisvandenbossche commented Oct 8, 2019

jbrockmendel Oct 8, 2019

Choose a reason for hiding this comment

jbrockmendel commented Oct 8, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 9, 2019

jorisvandenbossche commented Oct 11, 2019

jreback commented Oct 11, 2019

jorisvandenbossche commented Oct 11, 2019

jorisvandenbossche commented Oct 11, 2019

jbrockmendel Oct 11, 2019

Choose a reason for hiding this comment

jbrockmendel Oct 11, 2019

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback Oct 12, 2019

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 14, 2019

jreback commented Oct 14, 2019

TomAugspurger commented Oct 14, 2019 via email

jorisvandenbossche commented Oct 14, 2019

jorisvandenbossche commented Oct 15, 2019

lumberbot-app bot commented Oct 15, 2019

jorisvandenbossche commented Oct 15, 2019