BUG: conversion of empty DataFrame to SparseDtype (#33113) #33118

choucavalier · 2020-03-29T13:54:40Z

closes .astype(SparseDtype(float)) on empty dataframe leads to "ValueError: No objects to concatenate" #33113
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pandas/core/generic.py

jreback · 2020-03-29T14:54:45Z

rebase on master as have merged #33110 which might fix this (if so, you can just push the test itself)

choucavalier · 2020-03-29T15:05:21Z

@jreback now this is synced with master, i removed my changes in generic.py and only left the test, which is still failing.

jreback · 2020-03-29T15:30:07Z

@jreback now this is synced with master, i removed my changes in generic.py and only left the test, which is still failing.

then there maybe still an issue

simonjayhawkins · 2020-03-29T16:15:03Z

pandas/tests/frame/test_dtypes.py

@@ -552,6 +553,10 @@ def test_astype_to_incorrect_datetimelike(self, unit):
        with pytest.raises(TypeError, match=msg):
            df.astype(dtype)

+    def test_astype_sparsedtype_empty_dataframe(self):


since the same issue occurs with all extension array dtypes can you add a test to pandas\tests\extension\base\dtype.py instead.

choucavalier · 2020-03-29T20:18:11Z

@simonjayhawkins i've added the new test.

simonjayhawkins · 2020-03-30T09:03:44Z

pandas/tests/extension/base/dtype.py

+
+    def test_astype_empty_dataframe(self, dtype):
+        empty_dataframe = pd.DataFrame()
+        assert empty_dataframe.astype(dtype) == empty_dataframe


can you use tm.assert_frame_equal(result, empty_dataframe) here.

choucavalier · 2020-03-30T22:52:12Z

i've added a small fix by simply returning an empty dataframe when there's nothing to concatenate.

simonjayhawkins · 2020-03-31T08:46:22Z

Thanks @tgy can you add a whatsnew

choucavalier · 2020-03-31T16:13:40Z

@simonjayhawkins done!

choucavalier · 2020-03-31T20:53:22Z

@simonjayhawkins @jreback it seems that using tm.assert_frame_equal is prevented by CI checks

##[error]pandas/tests/extension/base/dtype.py:119: 
tm.assert_frame_equal(empty_dataframe.astype(dtype), empty_dataframe)

maybe i should only do empty_dataframe.astype(dtype) instead, without checking the result.

what do you recommend?

there's also a typing validation error:

pandas/core/generic.py:5573: error: 
Incompatible return value type (got "DataFrame", expected "FrameOrSeries")

what should i do about that?

simonjayhawkins · 2020-04-01T08:26:00Z

it seems that using tm.assert_frame_equal is prevented by CI checks
##[error]pandas/tests/extension/base/dtype.py:119: 
tm.assert_frame_equal(empty_dataframe.astype(dtype), empty_dataframe)
maybe i should only do empty_dataframe.astype(dtype) instead, without checking the result.

That was my mistake asking for the test to be moved in #33118 (comment). We'll probably need to keep the test where is was and use parameterization instead.

We don't have a fixture specifically for extension dtypes in conftest.py. (It maybe elsewhere), so could manually copy the dtypes from each of the /tests/extensions/.. tests for now.

simonjayhawkins · 2020-04-01T08:28:58Z

there's also a typing validation error:
pandas/core/generic.py:5573: error: 
Incompatible return value type (got "DataFrame", expected "FrameOrSeries")
what should i do about that?

The issue only affects DataFrame? but for mypy the return type needs to the same as the input type. maybe the original fix, see #33118 (comment), is more appropriate?

choucavalier · 2020-06-21T14:52:39Z

@jreback @simonjayhawkins I've updated the fix and moved the test. What do you guys think?

pep8speaks · 2020-06-21T18:47:34Z

Hello @tgy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-22 16:03:25 UTC

simonjayhawkins · 2020-06-21T18:47:35Z

Thanks @tgy for being patient here and sorry for reviewing sooner

it seems that using tm.assert_frame_equal is prevented by CI checks

ah my bad. should have been self.assert_frame_equal

i've moved the test (again!) to pandas\tests\extension\base\casting.py

can you add a release note to doc\source\whatsnew\v1.1.0.rst

…as into sparsedtype-conversion-empty-df

choucavalier · 2020-06-21T18:59:42Z

@simonjayhawkins done! I was the one who postponed that issue :-) Thanks for reviewing so quickly on a Sunday

simonjayhawkins

Thanks @tgy generally lgtm.

simonjayhawkins · 2020-06-21T19:49:16Z

doc/source/whatsnew/v1.1.0.rst

@@ -1068,6 +1068,7 @@ Sparse
 - Bug in :meth:`Series.sum` with ``SparseArray`` raises ``TypeError`` (:issue:`25777`)
 - Bug where :class:`DataFrame` containing :class:`SparseArray` filled with ``NaN`` when indexed by a list-like (:issue:`27781`, :issue:`29563`)
 - The repr of :class:`SparseDtype` now includes the repr of its ``fill_value`` attribute. Previously it used ``fill_value``'s  string representation (:issue:`34352`)
+- Fixed bug where :class:`DataFrame` or :class:`Series` could not be cast to :class:`SparseDtype` when empty (:issue:`33113`)


IIUC this fix is only for DataFrame. Series.astype works fine on master?

Good point. I tried and Series.astype works fine on master. Will fix the entry.

simonjayhawkins · 2020-06-21T19:49:27Z

pandas/core/generic.py

@@ -5548,6 +5548,9 @@ def astype(
            new_data = self._mgr.astype(dtype=dtype, copy=copy, errors=errors,)
            return self._constructor(new_data).__finalize__(self, method="astype")

+        if not results:


maybe this should be in elif is_extension_array_dtype(dtype) and self.ndim > 1: block.

although then the issue mentioned in #33113 (comment) would not be fixed. maybe keep this where it is and a test for #33113 (comment)

Sorry, did not see your second comment. Ok I'll do that. Anyhow if we reach the "concat" part of the code and results is empty, then we have a problem.

Although, one could argue that pd.concat of an empty list could return an empty frame or series instead of raising an exception.

@simonjayhawkins Actually starting to think the other issue was not an issue but an error on the author's part. See #33113 (comment). Awaiting your answer to proceed.

I think the other issue is relevant.

on master and in this PR (latest commit)...

>>> df = pd.DataFrame([[1, 2], [3, 4]]) >>> df 0 1 0 1 2 1 3 4 >>> >>> res = df.astype(dict()) >>> res 0 1 0 1 2 1 3 4 >>> >>> res is df False >>> >>> df = pd.DataFrame([dict(), dict()]) >>> >>> df.astype(dict()) Traceback (most recent call last): ... ValueError: No objects to concatenate >>>

this PR before moving #33118 (comment) ...

>>> df = pd.DataFrame([dict(), dict()]) >>> >>> df.astype(dict()) Empty DataFrame Columns: [] Index: [0, 1] >>> >>> res is df False

so it appears astype with an empty dict is a valid op with a copy returned. so this should also work on an empty DataFrame (even if created with empty dict).

these sort of circumstances can arise on data pipelines and having consistency reduces the complexity of downstream code.

so I think need to move the changes back and add a test for this.

Ok I will revert the changes back and add a test. I still find it very strange to call .astype(T) where T is an instance of an object and not a type (except of course for string-defined such as .astype("float64")).

DataFrame.astype can also accept a dict of column name -> data type as the dtype argument, https://pandas.pydata.org/docs/dev/reference/api/pandas.DataFrame.astype.html

Ahhh that makes a lot of sense now.

choucavalier · 2020-06-22T12:47:09Z

@simonjayhawkins let me know what you think :)

simonjayhawkins · 2020-06-22T14:54:46Z

@simonjayhawkins let me know what you think :)

see #33118 (comment)

simonjayhawkins · 2020-06-22T15:21:50Z

Thanks @tgy for adding the test. The test is not applicable to the extension array dtype tests. can you move it to pandas\tests\frame\methods\test_astype.py

simonjayhawkins · 2020-06-22T15:44:46Z

The test will still fail with name 'pd' is not defined and thereafter with AttributeError: 'TestAstype' object has no attribute 'assert_frame_equal'

you can run it locally

pytest pandas/tests/frame/methods/test_astype.py -k test_astype_empty_dtype_dict

can you also add assert result is not df

choucavalier · 2020-06-22T16:03:24Z

Thanks, should work now. Locally it did.

simonjayhawkins

Thanks @tgy lgtm @jreback

jreback · 2020-06-25T23:13:05Z

thanks @tgy

…andas-dev#33118)

jreback requested changes Mar 29, 2020

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode Sparse Sparse Data Type labels Mar 29, 2020

choucavalier mentioned this pull request Mar 29, 2020

.astype(SparseDtype(float)) on empty dataframe leads to "ValueError: No objects to concatenate" #33113

Closed

simonjayhawkins reviewed Mar 29, 2020

View reviewed changes

choucavalier changed the title ~~Fix conversion of empty DataFrame to SparseDtype (#33113)~~ BUG: Fix conversion of empty DataFrame to SparseDtype (#33113) Mar 30, 2020

choucavalier changed the title ~~BUG: Fix conversion of empty DataFrame to SparseDtype (#33113)~~ BUG: conversion of empty DataFrame to SparseDtype (#33113) Mar 30, 2020

simonjayhawkins reviewed Mar 30, 2020

View reviewed changes

simonjayhawkins added this to the 1.1 milestone Mar 31, 2020

simonjayhawkins self-requested a review May 8, 2020 16:37

choucavalier added 8 commits June 21, 2020 15:37

BUG: fix GH33113, conversion of empty DataFrame to SparseDtype

6ac82f8

return empty dataframe when nothing to concatenate

e76f1fc

revert black formatting

8a96a47

restore-to-master

a4aad14

return instance of class to handle frame or series

d42709b

add test

708fdc7

use type(self) instead of self.__class__

5b1fe9f

return self.copy()

738739e

choucavalier requested a review from jreback June 21, 2020 14:52

move test to extension array tests

a9e7e15

simonjayhawkins and others added 3 commits June 21, 2020 19:48

black fixup

9c7f06b

add whatsnew entry

49280cb

Merge branch 'sparsedtype-conversion-empty-df' of github.com:tgy/pand…

369735b

…as into sparsedtype-conversion-empty-df

simonjayhawkins reviewed Jun 21, 2020

View reviewed changes

address @simonjayhawkins's comments

f0f099c

revert back to previous solution + add test

dcdf11f

choucavalier added 2 commits June 22, 2020 17:28

move test

e2d4241

fix test

7bc6255

fix test again

95fd399

choucavalier requested a review from simonjayhawkins June 22, 2020 17:51

simonjayhawkins approved these changes Jun 22, 2020

View reviewed changes

jreback approved these changes Jun 25, 2020

View reviewed changes

jreback merged commit e37ff6e into pandas-dev:master Jun 25, 2020

fangchenli pushed a commit to fangchenli/pandas that referenced this pull request Jun 27, 2020

BUG: conversion of empty DataFrame to SparseDtype (pandas-dev#33113) (p…

c93a3b3

…andas-dev#33118)

BUG: conversion of empty DataFrame to SparseDtype (#33113) #33118

BUG: conversion of empty DataFrame to SparseDtype (#33113) #33118

Conversation

choucavalier commented Mar 29, 2020 • edited Loading

jreback commented Mar 29, 2020

choucavalier commented Mar 29, 2020

jreback commented Mar 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choucavalier commented Mar 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choucavalier commented Mar 30, 2020

simonjayhawkins commented Mar 31, 2020

choucavalier commented Mar 31, 2020

choucavalier commented Mar 31, 2020

simonjayhawkins commented Apr 1, 2020

simonjayhawkins commented Apr 1, 2020

choucavalier commented Jun 21, 2020

pep8speaks commented Jun 21, 2020 • edited Loading

Comment last updated at 2020-06-22 16:03:25 UTC

simonjayhawkins commented Jun 21, 2020

choucavalier commented Jun 21, 2020

simonjayhawkins 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

choucavalier Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choucavalier commented Jun 22, 2020

simonjayhawkins commented Jun 22, 2020

simonjayhawkins commented Jun 22, 2020

simonjayhawkins commented Jun 22, 2020 • edited Loading

choucavalier commented Jun 22, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

jreback commented Jun 25, 2020

choucavalier commented Mar 29, 2020 •

edited

Loading

pep8speaks commented Jun 21, 2020 •

edited

Loading

choucavalier Jun 22, 2020 •

edited

Loading

simonjayhawkins commented Jun 22, 2020 •

edited

Loading