Skip to content
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.

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

ENH: ExtensionArray.fillna #19909

Merged
merged 26 commits into from
Mar 16, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
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
66 changes: 66 additions & 0 deletions pandas/core/arrays/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,72 @@ def isna(self):
"""
raise AbstractMethodError(self)

def tolist(self):
# type: () -> list
"""Convert the array to a list of scalars."""
return list(self)
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed for fillna?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indirectly, via the default fillna.

Series.__iter__ calls Series.tolist, which calls Series._values.tolist. We could modify that to check the type and just call list(self._values) for EAs. I don't have a strong preference vs. adding a default .tolist.

Copy link
Member

Choose a reason for hiding this comment

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

Or directly call values.__iter__ ? As iterating through values to make a list and then iterate again through that list sounds like a bit of overhead.

For .tolist() itself, it is of course consistent with Series and numpy arrays, but personally I am not sure what its value is compared to list(values). I don't think you typically can implement a more efficient conversion to lists than what list(values) will do by default? (i.e. iterating through the values)


def fillna(self, value=None, method=None, limit=None):
""" Fill NA/NaN values using the specified method.

Parameters
----------
method : {'backfill', 'bfill', 'pad', 'ffill', None}, default None
Method to use for filling holes in reindexed Series
pad / ffill: propagate last valid observation forward to next valid
backfill / bfill: use NEXT valid observation to fill gap
value : scalar, array-like
Copy link
Member

Choose a reason for hiding this comment

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

value before method ? (order of signature)

If a scalar value is passed it is used to fill all missing values.
Alternatively, an array-like 'value' can be given. It's expected
that the array-like have the same length as 'self'.
limit : int, default None
(Not implemented yet for ExtensionArray!)
Copy link
Member

Choose a reason for hiding this comment

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

this can now maybe be passed to pad_1d / backfill_1d ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had to change things up slightly to use integers and the datetime fillna methods if you want to take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Categorical.fillna can probably do soemthing similar, haven't looked.

If method is specified, this is the maximum number of consecutive
NaN values to forward/backward fill. In other words, if there is
a gap with more than this number of consecutive NaNs, it will only
be partially filled. If method is not specified, this is the
maximum number of entries along the entire axis where NaNs will be
filled.

Returns
-------
filled : ExtensionArray with NA/NaN filled
"""
from pandas.api.types import is_scalar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A putmask type method would be extremely useful here.

I'll see what I can do to simplify this.

Copy link
Member

Choose a reason for hiding this comment

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

How would that be different with array[mask] = values ?

from pandas.util._validators import validate_fillna_kwargs
from pandas.core.missing import pad_1d, backfill_1d

value, method = validate_fillna_kwargs(value, method)

mask = self.isna()

if not is_scalar(value):
if len(value) != len(self):
raise ValueError("Length of 'value' does not match. Got ({}) "
" expected {}".format(len(value), len(self)))
value = value[mask]

if limit is not None:
msg = ("Specifying 'limit' for 'fillna' has not been implemented "
"yet for {} typed data".format(self.dtype))
raise NotImplementedError(msg)

if mask.any():
if method is not None:
# ffill / bfill
func = pad_1d if method == 'pad' else backfill_1d
Copy link
Member

Choose a reason for hiding this comment

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

is method normalized by here? (I mean, can it also be 'ffill'?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, by validate_fillna_kwargs

idx = np.arange(len(self), dtype=np.float64)
idx[mask] = np.nan
idx = func(idx, mask=mask).astype(np.int64)
Copy link
Member

Choose a reason for hiding this comment

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

probably needs to be np.intp (or an ensure_platform_int) for windows?

new_values = self.take(idx)
else:
# fill with value
new_values = self.copy()
new_values[mask] = value
else:
new_values = self.copy()
return type(self)(new_values)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait on #19906 before merging this, so I can update this line.

Copy link
Member

Choose a reason for hiding this comment

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

I think this can now just be new_values ?


# ------------------------------------------------------------------------
# Indexing methods
# ------------------------------------------------------------------------
Expand Down
38 changes: 17 additions & 21 deletions pandas/core/internals.py
Original file line number Diff line number Diff line change
Expand Up @@ -1963,6 +1963,23 @@ def concat_same_type(self, to_concat, placement=None):
return self.make_block_same_class(values, ndim=self.ndim,
placement=placement)

def fillna(self, value, limit=None, inplace=False, downcast=None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved from Categorical.fillna with changes

1.) Removed check for limit, since that's done in values.fillna
2.) Removed _try_coerce_result
For CategoricalBlock, this was unnecessary since
Categorical.fillna always returns a Categorical
3.) Used make_block_same_class
This limits ExtensionArray.fillna to not change the type
of the array / block, which I think is a good thing.

mgr=None):
values = self.values if inplace else self.values.copy()
values = values.fillna(value=value, limit=limit)
return [self.make_block_same_class(values=values,
placement=self.mgr_locs,
ndim=self.ndim)]

def interpolate(self, method='pad', axis=0, inplace=False, limit=None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just a move from CategoricalBlock to ExtensionBlock, no changes.

fill_value=None, **kwargs):

values = self.values if inplace else self.values.copy()
return self.make_block_same_class(
values=values.fillna(value=fill_value, method=method,
limit=limit),
placement=self.mgr_locs)


class NumericBlock(Block):
__slots__ = ()
Expand Down Expand Up @@ -2522,27 +2539,6 @@ def _try_coerce_result(self, result):

return result

def fillna(self, value, limit=None, inplace=False, downcast=None,
mgr=None):
# we may need to upcast our fill to match our dtype
if limit is not None:
raise NotImplementedError("specifying a limit for 'fillna' has "
"not been implemented yet")

values = self.values if inplace else self.values.copy()
values = self._try_coerce_result(values.fillna(value=value,
limit=limit))
return [self.make_block(values=values)]

def interpolate(self, method='pad', axis=0, inplace=False, limit=None,
fill_value=None, **kwargs):

values = self.values if inplace else self.values.copy()
return self.make_block_same_class(
values=values.fillna(fill_value=fill_value, method=method,
limit=limit),
placement=self.mgr_locs)

def shift(self, periods, axis=0, mgr=None):
return self.make_block_same_class(values=self.values.shift(periods),
placement=self.mgr_locs)
Expand Down
65 changes: 65 additions & 0 deletions pandas/tests/extension/base/missing.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import numpy as np
import pytest

import pandas as pd
import pandas.util.testing as tm
Expand Down Expand Up @@ -45,3 +46,67 @@ def test_dropna_frame(self, data_missing):
result = df.dropna()
expected = df.iloc[:0]
self.assert_frame_equal(result, expected)

def test_fillna_limit_raises(self, data_missing):
ser = pd.Series(data_missing)
fill_value = data_missing[1]
xpr = "Specifying 'limit' for 'fillna'.*{}".format(data_missing.dtype)

with tm.assert_raises_regex(NotImplementedError, xpr):
ser.fillna(fill_value, limit=2)

def test_fillna_series(self, data_missing):
fill_value = data_missing[1]
ser = pd.Series(data_missing)

result = ser.fillna(fill_value)
expected = pd.Series(type(data_missing)([fill_value, fill_value]))
self.assert_series_equal(result, expected)

# Fill with a series
result = ser.fillna(expected)
self.assert_series_equal(result, expected)

# Fill with a series not affecting the missing values
result = ser.fillna(ser)
self.assert_series_equal(result, ser)

@pytest.mark.parametrize('method', ['ffill', 'bfill'])
def test_fillna_series_method(self, data_missing, method):
fill_value = data_missing[1]

if method == 'ffill':
data_missing = type(data_missing)(data_missing[::-1])

result = pd.Series(data_missing).fillna(method=method)
expected = pd.Series(type(data_missing)([fill_value, fill_value]))

self.assert_series_equal(result, expected)

def test_fillna_frame(self, data_missing):
fill_value = data_missing[1]

result = pd.DataFrame({
"A": data_missing,
"B": [1, 2]
}).fillna(fill_value)

expected = pd.DataFrame({
"A": type(data_missing)([fill_value, fill_value]),
"B": [1, 2],
})

self.assert_frame_equal(result, expected)

def test_fillna_fill_other(self, data):
result = pd.DataFrame({
"A": data,
"B": [np.nan] * len(data)
}).fillna({"B": 0.0})

expected = pd.DataFrame({
"A": data,
"B": [0.0] * len(result),
})

self.assert_frame_equal(result, expected)
5 changes: 4 additions & 1 deletion pandas/tests/extension/category/test_categorical.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ def test_getitem_scalar(self):


class TestMissing(base.BaseMissingTests):
pass

@pytest.mark.skip(reason="Backwards compatability")
def test_fillna_limit_raises(self):
"""Has a different error message."""
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 can just up the error message for Categorical.fillna probably.



class TestMethods(base.BaseMethodsTests):
Expand Down
75 changes: 33 additions & 42 deletions pandas/tests/extension/decimal/test_decimal.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,68 +35,59 @@ def na_value():
return decimal.Decimal("NaN")


class TestDtype(base.BaseDtypeTests):
pass
class BaseDecimal(object):
@staticmethod
def assert_series_equal(left, right, *args, **kwargs):
# tm.assert_series_equal doesn't handle Decimal('NaN').
# We will ensure that the NA values match, and then
# drop those values before moving on.

left_na = left.isna()
right_na = right.isna()

class TestInterface(base.BaseInterfaceTests):
pass
tm.assert_series_equal(left_na, right_na)
tm.assert_series_equal(left[~left_na], right[~right_na],
*args, **kwargs)

@staticmethod
def assert_frame_equal(left, right, *args, **kwargs):
# TODO(EA): select_dtypes
decimals = (left.dtypes == 'decimal').index

class TestConstructors(base.BaseConstructorsTests):
pass
for col in decimals:
BaseDecimal.assert_series_equal(left[col], right[col],
*args, **kwargs)

left = left.drop(columns=decimals)
right = right.drop(columns=decimals)
tm.assert_frame_equal(left, right, *args, **kwargs)

class TestReshaping(base.BaseReshapingTests):

def test_align(self, data, na_value):
# Have to override since assert_series_equal doesn't
# compare Decimal(NaN) properly.
a = data[:3]
b = data[2:5]
r1, r2 = pd.Series(a).align(pd.Series(b, index=[1, 2, 3]))
class TestDtype(BaseDecimal, base.BaseDtypeTests):
pass

# NaN handling
e1 = pd.Series(type(data)(list(a) + [na_value]))
e2 = pd.Series(type(data)([na_value] + list(b)))
tm.assert_series_equal(r1.iloc[:3], e1.iloc[:3])
assert r1[3].is_nan()
assert e1[3].is_nan()

tm.assert_series_equal(r2.iloc[1:], e2.iloc[1:])
assert r2[0].is_nan()
assert e2[0].is_nan()
class TestInterface(BaseDecimal, base.BaseInterfaceTests):
pass

def test_align_frame(self, data, na_value):
# Override for Decimal(NaN) comparison
a = data[:3]
b = data[2:5]
r1, r2 = pd.DataFrame({'A': a}).align(
pd.DataFrame({'A': b}, index=[1, 2, 3])
)

# Assumes that the ctor can take a list of scalars of the type
e1 = pd.DataFrame({'A': type(data)(list(a) + [na_value])})
e2 = pd.DataFrame({'A': type(data)([na_value] + list(b))})
class TestConstructors(BaseDecimal, base.BaseConstructorsTests):
pass

tm.assert_frame_equal(r1.iloc[:3], e1.iloc[:3])
assert r1.loc[3, 'A'].is_nan()
assert e1.loc[3, 'A'].is_nan()

tm.assert_frame_equal(r2.iloc[1:], e2.iloc[1:])
assert r2.loc[0, 'A'].is_nan()
assert e2.loc[0, 'A'].is_nan()
class TestReshaping(BaseDecimal, base.BaseReshapingTests):
pass


class TestGetitem(base.BaseGetitemTests):
class TestGetitem(BaseDecimal, base.BaseGetitemTests):
pass


class TestMissing(base.BaseMissingTests):
class TestMissing(BaseDecimal, base.BaseMissingTests):
pass


class TestMethods(base.BaseMethodsTests):
class TestMethods(BaseDecimal, base.BaseMethodsTests):
@pytest.mark.parametrize('dropna', [True, False])
@pytest.mark.xfail(reason="value_counts not implemented yet.")
def test_value_counts(self, all_data, dropna):
Expand All @@ -112,7 +103,7 @@ def test_value_counts(self, all_data, dropna):
tm.assert_series_equal(result, expected)


class TestCasting(base.BaseCastingTests):
class TestCasting(BaseDecimal, base.BaseCastingTests):
pass


Expand Down
8 changes: 7 additions & 1 deletion pandas/tests/extension/json/test_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@ class TestGetitem(base.BaseGetitemTests):


class TestMissing(base.BaseMissingTests):
pass
@pytest.mark.xfail(reason="Setting a dict as a scalar")
def test_fillna_series(self):
"""We treat dictionaries as a mapping in fillna, not a scalar."""

@pytest.mark.xfail(reason="Setting a dict as a scalar")
def test_fillna_frame(self):
"""We treat dictionaries as a mapping in fillna, not a scalar."""


class TestMethods(base.BaseMethodsTests):
Expand Down