Skip to content

ENH: ExtensionArray.fillna #19909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Mar 16, 2018
Merged

ENH: ExtensionArray.fillna #19909

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
74614cb
ENH: ExtensionArray.fillna
TomAugspurger Feb 22, 2018
67a19ba
REF: cast to object
TomAugspurger Feb 27, 2018
280ac94
Revert "REF: cast to object"
TomAugspurger Feb 27, 2018
6705712
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Feb 28, 2018
69a0f9b
Simpler implementation
TomAugspurger Feb 28, 2018
4d6846b
Support limit
TomAugspurger Feb 28, 2018
f3b81dc
Test backfill with limit
TomAugspurger Feb 28, 2018
70efbb8
BUG: ensure array-like for indexer
TomAugspurger Feb 28, 2018
8fc3851
Refactor tolist
TomAugspurger Mar 1, 2018
39096e5
Test Series[ea].tolist
TomAugspurger Mar 1, 2018
9e44f88
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 1, 2018
e902f18
Fixed Categorical unwrapping
TomAugspurger Mar 1, 2018
17126e6
updated
TomAugspurger Mar 2, 2018
1106ef2
Back to getvalues list
TomAugspurger Mar 2, 2018
744c381
Simplified
TomAugspurger Mar 2, 2018
9a3fa55
As a method
TomAugspurger Mar 2, 2018
2bc43bc
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 3, 2018
f22fd7b
Removed tolist fully
TomAugspurger Mar 3, 2018
c26d261
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 5, 2018
b342efe
Linting
TomAugspurger Mar 5, 2018
a609f48
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 12, 2018
a35f93c
Just apply to objects
TomAugspurger Mar 12, 2018
3f78dec
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 12, 2018
1160e15
Linting
TomAugspurger Mar 13, 2018
c9c7a48
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 14, 2018
05fced6
Merge remote-tracking branch 'upstream/master' into fu1+fillna
TomAugspurger Mar 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.23.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,7 @@ Categorical
- Bug in :meth:`Series.astype` and ``Categorical.astype()`` where an existing categorical data does not get updated (:issue:`10696`, :issue:`18593`)
- Bug in :class:`Index` constructor with ``dtype=CategoricalDtype(...)`` where ``categories`` and ``ordered`` are not maintained (issue:`19032`)
- Bug in :class:`Series` constructor with scalar and ``dtype=CategoricalDtype(...)`` where ``categories`` and ``ordered`` are not maintained (issue:`19565`)
- Bug in ``Categorical.__iter__`` not converting to Python types (:issue:`19909`)

Datetimelike
^^^^^^^^^^^^
Expand Down
9 changes: 4 additions & 5 deletions pandas/core/arrays/categorical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
from pandas.core.dtypes.missing import isna, notna
from pandas.core.dtypes.cast import (
maybe_infer_to_datetimelike,
coerce_indexer_dtype)
coerce_indexer_dtype,
tolist)
from pandas.core.dtypes.dtypes import CategoricalDtype
from pandas.core.dtypes.common import (
_ensure_int64,
Expand Down Expand Up @@ -475,9 +476,7 @@ def tolist(self):
(for str, int, float) or a pandas scalar
(for Timestamp/Timedelta/Interval/Period)
"""
if is_datetimelike(self.categories):
return [com._maybe_box_datetimelike(x) for x in self]
return np.array(self).tolist()
return tolist(self)

@property
def base(self):
Expand Down Expand Up @@ -1712,7 +1711,7 @@ def __len__(self):

def __iter__(self):
"""Returns an Iterator over the values of this Categorical."""
return iter(self.get_values())
return iter(self.get_values().tolist())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this does self.get_values().tolist() instead of directly self.tolist() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to avoid an infinite loop.

Categorical.tolist calls tolist which calls Categorical.__iter__.

Need to do get_values break that cycle / extract the python scalar types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just feels that we are doing to much work. To iterate, we first create an array, iterate through it to create a list, and then iterate through the list ..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So get_values either returns a array or a Datetime(like)Index. For the array we can just iterate over it, but for the Datetime(like)Index I think as well . The tolist implementation there is list(self.astype(object))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that previous comment was of course wrong, as that is exactly checking the get_values().tolist()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, therefore we need the tolist.
But then moving the logic is maybe not best, as then tolist would consume an iterator coming from a list ..

It's all just complex with the different unboxing depending on the type :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified slightly if you want to take one last look. Basically, we didn't have to worry about the different unboxing for different types, since Categorical.get_values() with datetimes returns a DatetimeIndex, and when we .tolist() on that we get the right unboxing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the whole .get_values() fiasco (which I created a while back :). I think should be addressed sooner rather than later.

Copy link
Contributor Author

@TomAugspurger TomAugspurger Mar 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any issues with the current implementation? Currently there's a tradeoff between how expensive it = iter(Categorical) is vs. next(it).

Somewhere, we have to go from numpy scalars to Python scalars. The fastest way to do that is with tolist(), but that has upfront memory overhead. We could avoid that by doing it in the next, but that has time overhead for

  1. converting to Python scalars elementwise
  2. An extra call to CategoricalIndex.categories.getitem per element

so that next(it) becomes much slower. I don't think there's much more to be done right now.


def _tidy_repr(self, max_vals=10, footer=True):
""" a short repr displaying only max_vals and an optional (but default
Expand Down
5 changes: 5 additions & 0 deletions pandas/tests/categorical/test_dtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,8 @@ def test_astype_category(self, dtype_ordered, cat_ordered):
result = cat.astype('category')
expected = cat
tm.assert_categorical_equal(result, expected)

def test_iter_python_types(self):
# GH-19909
cat = Categorical([1, 2])
assert isinstance(list(cat)[0], int)