Skip to content

BUG: Fix construction of Categorical from pd.NA #31939

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 24 commits into from
Feb 23, 2020
Merged

BUG: Fix construction of Categorical from pd.NA #31939

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
99dbff4
Add test with NA
Feb 12, 2020
81516a6
Add GH issue
Feb 12, 2020
78d62f9
Check for NA
Feb 12, 2020
38fede6
Update whatsnew
Feb 12, 2020
52466ab
Add MultiIndex test
Feb 12, 2020
1a71728
whatsnew for MultiIndex
Feb 12, 2020
bad5be3
Parametrize test
Feb 12, 2020
b051bf0
Use pd.NA
Feb 12, 2020
563b673
Import C_NA
Feb 13, 2020
9066789
Merge branch 'master' into na-cat
Feb 15, 2020
7da4e44
Edit release note
Feb 16, 2020
d1a953b
Align conditions
Feb 16, 2020
baab1d5
Construct from tuples in test
Feb 16, 2020
062f5f7
Merge branch 'master' into na-cat
Feb 16, 2020
2d45b21
Add a string
Feb 16, 2020
14a737d
Use checknull
Feb 17, 2020
f0eb9f3
Merge branch 'master' into na-cat
Feb 17, 2020
a54fe0d
Revert "Use checknull"
Feb 17, 2020
17de660
Merge branch 'master' into na-cat
Feb 17, 2020
78e38ec
Update doc/source/whatsnew/v1.0.2.rst
dsaxton Feb 18, 2020
0efcdb0
Take out MultiIndex stuff
Feb 19, 2020
a04df9b
Merge branch 'master' into na-cat
dsaxton Feb 19, 2020
3c5082e
Merge remote-tracking branch 'upstream/master' into na-cat
dsaxton Feb 23, 2020
d50f963
Parametrize and hard code
dsaxton Feb 23, 2020
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
5 changes: 5 additions & 0 deletions doc/source/whatsnew/v1.0.2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ Bug fixes
**Categorical**

- Fixed bug where :meth:`Categorical.from_codes` improperly raised a ``ValueError`` when passed nullable integer codes. (:issue:`31779`)
- Fixed bug where :meth:`Categorical` constructor would raise a ``TypeError`` when given a numpy array containing ``NA``. (:issue:`31927`)

**MultiIndex**

- Fixed bug where :meth:`MultiIndex.from_product` would break when handling numpy object arrays containing ``NA``. (:issue:`31883`)

**I/O**

Expand Down
4 changes: 3 additions & 1 deletion pandas/_libs/hashtable_class_helper.pxi.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ WARNING: DO NOT edit .pxi FILE directly, .pxi is generated from .pxi.in
# ----------------------------------------------------------------------

from pandas._libs.tslibs.util cimport get_c_string
from pandas._libs.missing cimport C_NA

{{py:

Expand Down Expand Up @@ -1032,7 +1033,8 @@ cdef class PyObjectHashTable(HashTable):
val = values[i]
hash(val)

if ignore_na and ((val != val or val is None)
if ignore_na and ((val is C_NA)
or (val != val or val is None)
or (use_na_value and val == na_value)):
# if missing values do not count as unique values (i.e. if
# ignore_na is True), skip the hashtable entry for them, and
Expand Down
8 changes: 8 additions & 0 deletions pandas/tests/arrays/categorical/test_constructors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,14 @@ def test_constructor_with_categorical_categories(self):
result = Categorical(["a", "b"], categories=CategoricalIndex(["a", "b", "c"]))
tm.assert_categorical_equal(result, expected)

def test_construction_with_null(self, nulls_fixture):
# https://github.com/pandas-dev/pandas/issues/31927
values = ["a", nulls_fixture]
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add another value after nulls_fixture here to show that we are actually getting the correct codes

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand this request. I did just realize though that pd.NA is not even part of nulls_fixture so will need to change this somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is, rebase on master

Copy link
Contributor

Choose a reason for hiding this comment

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

values = ['a', nulls_fixture, 'b']

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I looking at the wrong fixture? I wasn't seeing pd.NA here: https://github.com/pandas-dev/pandas/blob/master/pandas/conftest.py#L444

Copy link
Member

Choose a reason for hiding this comment

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

Will be added in #31799

result = Categorical(np.array(values, dtype=object))
expected = Categorical(values)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is a very good test. I mean: it is testing that lists vs object array are giving the same result (which is useful anyhow, as those should be consistent), but it is not testing how they are now constructed (eg it won't "preserve" pd.NA, and this is also not tested)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dsaxton can you parameterize this on klass (np.array and list), then hard code the results in a categorical (meaning use _from_codes and an explict list of categories)


tm.assert_categorical_equal(result, expected)

def test_from_codes(self):

# too few categories
Expand Down
10 changes: 10 additions & 0 deletions pandas/tests/indexing/multiindex/test_multiindex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,13 @@ def test_nested_tuples_duplicates(self):
df3 = df.copy(deep=True)
df3.loc[[(dti[0], "a")], "c2"] = 1.0
tm.assert_frame_equal(df3, expected)

def test_multiindex_from_product_contains_na(self):
# https://github.com/pandas-dev/pandas/issues/31883
values1 = [np.array([0.0, pd.NA], dtype="object"), ["a", "b"]]
values2 = [np.array([0.0, np.nan], dtype="object"), ["a", "b"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, pd.NA is actually converted to np.nan here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, probably not ideal (but better than an error). If merged would a follow-up issue to make sure pd.NA is used make sense? Or I could mark that the referenced issue is not actually closed and comment there.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, pls do it here

Copy link
Contributor

Choose a reason for hiding this comment

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

followups are ok, but for relatively small things just fixing it in the same PR is better

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd have to look more closely, but I'm not sure if having it return pd.NA instead of np.nan is an easy fix; this is already how it behaves for list input (which seems to be the documented behavior):

In [1]: import pandas as pd 
   ...:  
   ...: values = ["a", pd.NA] 
   ...:  
   ...: pd.Categorical(values) 
   ...:                                                                                                                            
Out[1]: 
[a, NaN]
Categories (1, object): [a]

In [2]: pd.__version__                                                                                                             
Out[2]: '1.0.1'

I think having it so that we at least get the same output and not an error for a numpy array with object dtype is still an improvement though? What are your thoughts @WillAyd ?

Copy link
Member

Choose a reason for hiding this comment

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

Agree it would be nice to maintain pd.NA - do you know the extra effort involved to do so?

Copy link
Member Author

@dsaxton dsaxton Feb 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure, I'd need to investigate a bit more. The logic doesn't seem too obvious though; should pd.NA be the default for missing values, or only when it's explicitly encountered? How should mixed missing value types get handled during construction (e.g., pd.Categorical(["a", np.nan, pd.NA]))? Personally I think having pd.NA as the default makes sense, but that seems like a large change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Curious if @jorisvandenbossche has a preference? Always using pd.NA seems logical, probably just a question of "when" to implement that (as a lot of people are likely using Categorical and expecting to see NaN).


result = pd.MultiIndex.from_product(values1)
expected = pd.MultiIndex.from_product(values2)

tm.assert_index_equal(result, expected)