ENH: ExtensionArray.fillna
State: closed

xref https://github.com/pandas-dev/pandas/issues/19696

This adds `ExtensionArray.fillna`, and support for `NDFrame.fillna` with extension arrays.

Like `Categorical.fillna`, `limit` is not implemented. This shouldn't make it any harder to support that in the future.

Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=h1) Report
> Merging [#19909](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/38afa9310040f1bd4fb122008e96fe6d719b12a2?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `93.54%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/19909/graphs/tree.svg?width=650&src=pr&token=eZ4WkYLtcO&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #19909      +/-   ##
==========================================
- Coverage   91.78%   91.75%   -0.03%     
==========================================
  Files         152      152              
  Lines       49196    49214      +18     
==========================================
+ Hits        45153    45155       +2     
- Misses       4043     4059      +16
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.13% <93.54%> (-0.03%)` | :arrow_down: |
| #single | `41.81% <22.58%> (-0.02%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `87.68% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.09% <100%> (-0.02%)` | :arrow_down: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `95.53% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYmFzZS5weQ==) | `96.78% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `80.64% <89.47%> (+3.9%)` | :arrow_up: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.07% <0%> (-1.74%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/19909/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `83.74% <0%> (-0.21%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=footer). Last update [38afa93...05fced6](https://codecov.io/gh/pandas-dev/pandas/pull/19909?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jreback @jorisvandenbossche do you have a preference for `tolist`? Here's the diff removing it.

```diff
diff --git a/pandas/core/arrays/base.py b/pandas/core/arrays/base.py
index 8dc4ddbff..bb58fefe5 100644
--- a/pandas/core/arrays/base.py
+++ b/pandas/core/arrays/base.py
@@ -218,11 +218,6 @@ class ExtensionArray(object):
         """
         raise AbstractMethodError(self)
 
-    def tolist(self):
-        # type: () -> list
-        """Convert the array to a list of scalars."""
-        return list(self)
-
     def fillna(self, value=None, method=None, limit=None):
         """ Fill NA/NaN values using the specified method.
 
diff --git a/pandas/core/base.py b/pandas/core/base.py
index 280b88497..5ced4ec25 100644
--- a/pandas/core/base.py
+++ b/pandas/core/base.py
@@ -829,6 +829,8 @@ class IndexOpsMixin(object):
 
         if is_datetimelike(self):
             return [com._maybe_box_datetimelike(x) for x in self._values]
+        elif is_extension_array_dtype(self):
+            return list(self._values)
         else:
             return self._values.tolist()
```

I don't have a strong preference, but slightly lean towards implementing it (the current state of the PR). It's ugly, but practical in this case.
Updated to remove `tolist` for now if you could take another look @jorisvandenbossche.

Added a new method `tolist` to cast.py that basically does that. I didn't see any other places we could use it, other than `IndexOpsMixin.tolist`.
Good call.

On Thu, Mar 1, 2018 at 2:31 PM, Joris Van den Bossche <
notifications@github.com> wrote:

> *@jorisvandenbossche* commented on this pull request.
> ------------------------------
>
> In pandas/tests/extension/base/interface.py
> <https://github.com/pandas-dev/pandas/pull/19909#discussion_r171684925>:
>
> > @@ -53,3 +54,8 @@ def test_is_extension_array_dtype(self, data):
>          assert is_extension_array_dtype(data.dtype)
>          assert is_extension_array_dtype(pd.Series(data))
>          assert isinstance(data.dtype, ExtensionDtype)
> +
> +    def test_tolist(self, data):
> +        result = tolist(data)
>
> maybe rather test Series[extension].tolist() here? to check it is the same
> as list(data) (instead of testing this internal method)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/19909#pullrequestreview-100582449>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIklIZF7WQfxJSi7gMtcRQFeI4_OTks5taFqNgaJpZM4SThPt>
> .
>

This is fine for me. I agree it is a bit more complexity, so if somebody feels strong about it, I won't object having a `tolist` (but still, I think it is better to keep the interface as limited as possible for now, and `tolist` feels a bit superfluous from the extension array point of view).
This turned up an issue with `Categorical.__iter__` not unwrapping NumPy scalars to their Python types. I fixed that in the latest commit, to be consistent with the rest of pandas `__iter__` methods.
Agreed. Let me see if there's a way that has fewer intermediate allocations.

On Fri, Mar 2, 2018 at 6:48 AM, Joris Van den Bossche <
notifications@github.com> wrote:

> *@jorisvandenbossche* commented on this pull request.
> ------------------------------
>
> In pandas/core/arrays/categorical.py
> <https://github.com/pandas-dev/pandas/pull/19909#discussion_r171838479>:
>
> > @@ -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())
>
> 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 ..
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/19909#discussion_r171838479>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIlj1F_0zxgqQ4Hc_hm9rNqXD88xPks5taT-IgaJpZM4SThPt>
> .
>

> I am still not clear why you are writing another implementation of fill here. We already have one for object dtypes. I would suggest using that one. Yes I get that you want one for the extension classes, but if that is the case then I would replace the algos we have with THIS one. There is no reason to have 2 floating around. Then this will be fully tested and you can look at performance and such.

Sorry, but I really disagree that having a *good* implementation of `fillna` for ExtensionArrays is out of scope. 
The implementation we had before is really not that trivial for external authors to do themselves, and was using internal API. But it was a good generic implementation that would work in many cases.
But OK, let's go ahead with this PR, we can always discuss later an improved implementation
Ah I opened https://github.com/pandas-dev/pandas/issues/20300

I don't really care whether we do it now or elsewhere. That type of filling on integer, followed by a take seems like a useful strategy in general. I can do a followup implementing in in `core/missing.py` and the call it here.
OK, let's merge this one then!
i will have a look later today 
Thanks. No rush, I'm probably reviewing docs all day. Just wanted to make
sure my PRs were passing on master.

On Mon, Mar 12, 2018 at 9:23 AM, Jeff Reback <notifications@github.com>
wrote:

> i will have a look later today
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/19909#issuecomment-372326949>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIknkp4OrXDJqabfbYDkxusnhLLqlks5tdoTzgaJpZM4SThPt>
> .
>

Fixed a merge conflict.

This should be ready as well.
Fixed the merge conflict.
All green.
you never answered my question of why you are adding a *new* implementation and not using the current one.
I removed the new implementation. https://github.com/pandas-dev/pandas/issues/20300
Was that your last objection?
thanks @TomAugspurger, no didn't see the removal, but looking good now