BUG: Fix Categorical use_inf_as_na bug #33629

dsaxton · 2020-04-18T14:25:58Z

closes BUG: pandas 1.0 dropna error with categorical data if pd.options.mode.use_inf_as_na = True #33594
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pandas/core/dtypes/missing.py

WillAyd

jreback · 2020-04-19T02:06:01Z

pandas/core/dtypes/missing.py

@@ -231,7 +231,11 @@ def _isna_ndarraylike(obj):


 def _isna_ndarraylike_old(obj):
-    values = getattr(obj, "_values", obj)
+    if not isinstance(obj, np.ndarray):


is the the same construct we use in isna_arraylike?

In _isna_arraylike it looks like we use getattr, but then check if we have an ExtensionArray and call the isna method if so

I agree the code should use the same structure as _isna_ndarraylike, refactoring out the common code maybe advantageous to avoid divergence occurring again.

How would you recommend using a similar pattern here? We can't rely on isna to catch infinity (it only works if you construct the array after setting the option):

[ins] In [1]: cat = pd.Categorical([1, 2, np.inf]) [ins] In [2]: pd.options.mode.use_inf_as_na = True [ins] In [3]: cat.isna() Out[3]: array([False, False, False]) [ins] In [4]: cat = pd.Categorical([1, 2, np.inf]) [ins] In [5]: cat.isna() Out[5]: array([False, False, True])

so is there also a bug in Categorical.isna?

I suppose you could call this behavior a bug, but I'm not sure if it's a problem with Categorical.isna (it's simply checking Categorical._codes, which makes total sense and shouldn't need to change in my opinion). IMHO the issue is more with trying to have the behavior of these constructors / methods respect hidden global variables instead of being well-defined in isolation (I could even see the merit of deprecating options like this rather than needing to patch every place where NA checking might happen)

I would prefer to make this code substatially the same, difference only be the inf change.

pandas/_libs/missing.pyx

simonjayhawkins · 2020-04-19T10:24:04Z

pandas/_libs/missing.pyx

@@ -160,7 +161,7 @@ def isnaobj_old(arr: ndarray) -> ndarray:
    result = np.zeros(n, dtype=np.uint8)
    for i in range(n):
        val = arr[i]
-        result[i] = val is NaT or _check_none_nan_inf_neginf(val)
+        result[i] = checknull(val) or val == INF or val == NEGINF


Is this related to the OP in #33594?. I think the NA checking for string dtype #33594 (comment) maybe should be a separate issue/PR. We may want to backport the fix relating to the change in return type of Categorical.ravel from numpy array to Categorical.

It's unrelated, I found this other bug while debugging the original issue and decided to fix it here. I'll go ahead and separate it out for easier bookkeeping.

Moved to #33656

Actually, they are kind of related in that a fix needs to be made to missing.py as well. I'll wait to see how that ends up here before making the associated change in the other PR.

one is a bug in _isna_ndarraylike_old and the other is a bug in isnaobj_old. They are related but I think should be treated indepedently.

for the NA issue (bug in isnaobj_old once bug in _isna_ndarraylike_old is fixed) we may want to add more tests for other extension arrays or add a test/parameterise an existing test in the base extension array tests.

jreback · 2020-04-19T21:25:42Z

pandas/core/dtypes/missing.py

@@ -231,7 +231,11 @@ def _isna_ndarraylike(obj):


 def _isna_ndarraylike_old(obj):
-    values = getattr(obj, "_values", obj)
+    if not isinstance(obj, np.ndarray):


I would prefer to make this code substatially the same, difference only be the inf change.

TomAugspurger · 2020-04-20T11:26:49Z

pandas/core/dtypes/missing.py

@@ -233,9 +233,10 @@ def _isna_ndarraylike_old(obj):
    values = getattr(obj, "_values", obj)
    dtype = values.dtype

-    if is_string_dtype(dtype):
+    if is_extension_array_dtype(dtype):
+        result = values.isna() | (values == -np.inf) | (values == np.inf)


Is this always treating inf as NA?

Yes, I think / hope that's the intention when this gets called (e.g., at line 244 we have result = ~np.isfinite(values))

can you make the similar code paths in isna_ndarraylike an here

The functions are almost the same now, unless there are objections (can revert if so) I'm going to try parametrizing _isna_ndarraylike over old / new and just deleting this function

jorisvandenbossche · 2020-04-20T13:08:15Z

pandas/tests/arrays/categorical/test_missing.py

+    def test_use_inf_as_na(self, values, expected):
+        # https://github.com/pandas-dev/pandas/issues/33594
+        with pd.option_context("mode.use_inf_as_na", True):
+            cat = Categorical(values)


Should we also test this with putting the Categorical creation outside of the option context?

It turns out that actually fails interestingly enough: #33629 (comment) (if you set the option after constructing the object it loses its effect). What's even more strange is the value displays as NaN:

[ins] In [3]: arr = pd.Categorical([1, 2, np.inf]) [ins] In [4]: pd.options.mode.use_inf_as_na = True [ins] In [5]: arr Out[5]: [1.0, 2.0, NaN] Categories (3, float64): [1.0, 2.0, NaN] [ins] In [6]: arr.isna() Out[6]: array([False, False, False])

What's even more strange is the value displays as NaN:

I think that is somewhat "expected", given the discussion above in #33629 (comment) (not that I like that behaviour though).

But regardless of how the categorical is created (with or without the option), I find it very strange that isna is then failing

Hmm, being a bit confused in the comment above :)
The Categorical.isna is failing, of course, since it just looks at -1 in the codes, and when inf is part of the categories, it is not -1 in the codes. Hence it is failing to detect it.

@dsaxton so could you additionally test it for pd.isna ? In constract to Categorical.isna(), pd.isna should work even if the Categorical is created before the context, I think?

@jorisvandenbossche Added some tests, let me know if this is roughly what you had in mind

Yes, that looks good!

jreback · 2020-04-21T13:30:11Z

pandas/core/dtypes/missing.py

@@ -233,9 +233,10 @@ def _isna_ndarraylike_old(obj):
    values = getattr(obj, "_values", obj)
    dtype = values.dtype

-    if is_string_dtype(dtype):
+    if is_extension_array_dtype(dtype):
+        result = values.isna() | (values == -np.inf) | (values == np.inf)


can you make the similar code paths in isna_ndarraylike an here

pandas/tests/arrays/categorical/test_missing.py

jreback

small change, otherwise lgtm. ping on green.

in a followon, I'd like to remove entirely the _isna_old and push just have a single parameterized one as you can just parameterize _isna_new / _old

jreback · 2020-04-26T20:19:26Z

pandas/core/dtypes/missing.py

@@ -207,40 +207,25 @@ def _use_inf_as_na(key):
        globals()["_isna"] = _isna_new


-def _isna_ndarraylike(obj):
+def _isna_ndarraylike(obj, old=False):


can you add a doc-string here and type old

jreback · 2020-04-27T21:07:43Z

thanks @dsaxton

if you can push the followon when you can to consolidate code here would be great

simonjayhawkins · 2020-05-05T08:45:58Z

xref #33300 to track.

…e_inf_as_na bug)

…a bug) (#34001) Co-authored-by: Daniel Saxton <2658661+dsaxton@users.noreply.github.com>

BUG: Fix use_inf_as_na bugs

8985831

dsaxton added Bug Categorical Categorical Data Type Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Apr 18, 2020

dsaxton added 2 commits April 18, 2020 09:29

Note

55ed2cf

Issue number

9582927

dsaxton commented Apr 18, 2020

View reviewed changes

pandas/core/dtypes/missing.py Outdated Show resolved Hide resolved

Oops

365df7e

WillAyd reviewed Apr 19, 2020

View reviewed changes

jreback requested changes Apr 19, 2020

View reviewed changes

dsaxton added 2 commits April 18, 2020 21:38

Different check

1d05145

Merge remote-tracking branch 'upstream/master' into use-inf-as-na

a1b12ba

simonjayhawkins reviewed Apr 19, 2020

View reviewed changes

dsaxton added 2 commits April 19, 2020 10:48

Remove StringArray stuff

c7c03b8

Fix

dbf9830

dsaxton changed the title ~~BUG: Fix use_inf_as_na bugs~~ BUG: Fix Categorical use_inf_as_na bug Apr 19, 2020

dsaxton removed the NA - MaskedArrays Related to pd.NA and nullable extension arrays label Apr 19, 2020

jreback requested changes Apr 19, 2020

View reviewed changes

jreback mentioned this pull request Apr 19, 2020

BUG: Fix StringArray use_inf_as_na bug #33656

Merged

5 tasks

dsaxton added 3 commits April 19, 2020 20:32

Update

60c7625

Merge remote-tracking branch 'upstream/master' into use-inf-as-na

6b413b4

Use equality instead

ddff3d2

TomAugspurger reviewed Apr 20, 2020

View reviewed changes

jorisvandenbossche approved these changes Apr 20, 2020

View reviewed changes

jorisvandenbossche reviewed Apr 20, 2020

View reviewed changes

jreback requested changes Apr 21, 2020

View reviewed changes

dsaxton added 3 commits April 21, 2020 14:13

Delete _isna_ndarraylike_old

cbfdd6a

Merge remote-tracking branch 'upstream/master' into use-inf-as-na

81c310f

Add some tests

1838a33

jorisvandenbossche reviewed Apr 21, 2020

View reviewed changes

pandas/tests/arrays/categorical/test_missing.py Outdated Show resolved Hide resolved

Fix test

11d50a9

jorisvandenbossche approved these changes Apr 24, 2020

View reviewed changes

jreback requested changes Apr 26, 2020

View reviewed changes

jreback added this to the 1.1 milestone Apr 26, 2020

dsaxton added 4 commits April 26, 2020 17:08

Add docstring

afa9448

Merge remote-tracking branch 'upstream/master' into use-inf-as-na

c7b65d5

Fix

a95f38c

Nit

fe1648f

jreback approved these changes Apr 27, 2020

View reviewed changes

jreback merged commit 91150d9 into pandas-dev:master Apr 27, 2020

dsaxton deleted the use-inf-as-na branch April 27, 2020 21:40

simonjayhawkins pushed a commit to simonjayhawkins/pandas that referenced this pull request May 5, 2020

Backport PR pandas-dev#33629 on branch 1.0.x (BUG: Fix Categorical us…

fad6a52

…e_inf_as_na bug)

simonjayhawkins mentioned this pull request May 5, 2020

Backport PR #33629 on branch 1.0.x (BUG: Fix Categorical use_inf_as_n… #34001

Merged

simonjayhawkins added a commit that referenced this pull request May 5, 2020

Backport PR #33629 on branch 1.0.x (BUG: Fix Categorical use_inf_as_n…

60e47f5

…a bug) (#34001) Co-authored-by: Daniel Saxton <2658661+dsaxton@users.noreply.github.com>

rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020

BUG: Fix Categorical use_inf_as_na bug (pandas-dev#33629)

9790962

simonjayhawkins modified the milestones: 1.1, 1.0.4 May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix Categorical use_inf_as_na bug #33629

BUG: Fix Categorical use_inf_as_na bug #33629

dsaxton commented Apr 18, 2020 •

edited

Loading

WillAyd left a comment

jreback Apr 19, 2020

dsaxton Apr 19, 2020 •

edited

Loading

simonjayhawkins Apr 19, 2020

dsaxton Apr 19, 2020

simonjayhawkins Apr 19, 2020

dsaxton Apr 19, 2020

jreback Apr 19, 2020

simonjayhawkins Apr 19, 2020

dsaxton Apr 19, 2020

dsaxton Apr 19, 2020

dsaxton Apr 19, 2020 •

edited

Loading

simonjayhawkins Apr 19, 2020

jreback Apr 19, 2020

TomAugspurger Apr 20, 2020

dsaxton Apr 20, 2020

jreback Apr 21, 2020

dsaxton Apr 21, 2020 •

edited

Loading

jorisvandenbossche Apr 20, 2020

dsaxton Apr 20, 2020 •

edited

Loading

jorisvandenbossche Apr 20, 2020

jorisvandenbossche Apr 20, 2020 •

edited

Loading

jorisvandenbossche Apr 21, 2020

dsaxton Apr 21, 2020

jorisvandenbossche Apr 21, 2020

jreback Apr 21, 2020

jreback left a comment

jreback Apr 26, 2020

jreback commented Apr 27, 2020

simonjayhawkins commented May 5, 2020

BUG: Fix Categorical use_inf_as_na bug #33629

BUG: Fix Categorical use_inf_as_na bug #33629

Conversation

dsaxton commented Apr 18, 2020 • edited Loading

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsaxton Apr 19, 2020 • edited Loading

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

dsaxton Apr 19, 2020 • edited Loading

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

dsaxton Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsaxton Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche Apr 20, 2020 • edited Loading

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

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Apr 27, 2020

simonjayhawkins commented May 5, 2020

dsaxton commented Apr 18, 2020 •

edited

Loading

dsaxton Apr 19, 2020 •

edited

Loading

dsaxton Apr 19, 2020 •

edited

Loading

dsaxton Apr 21, 2020 •

edited

Loading

dsaxton Apr 20, 2020 •

edited

Loading

jorisvandenbossche Apr 20, 2020 •

edited

Loading