Skip to content

BUG: Avoid casting Int to object in Categorical.from_codes #31794

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 11 commits into from
Feb 12, 2020
Merged

BUG: Avoid casting Int to object in Categorical.from_codes #31794

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
416a869
Add test
Feb 7, 2020
4bd7588
Don't cast EA to object
Feb 7, 2020
6c2edda
Add whatsnew
Feb 7, 2020
916998f
Merge branch 'master' into cat-from-int-codes
Feb 8, 2020
270199a
Update pandas/tests/arrays/categorical/test_constructors.py
dsaxton Feb 8, 2020
75e25d6
No na_value
Feb 8, 2020
acbadaa
Merge branch 'master' into cat-from-int-codes
Feb 8, 2020
6ec804e
Remove issue number comment
Feb 9, 2020
c5508dc
Add test with NA code
Feb 9, 2020
63b7c1c
Raise a better error message
Feb 11, 2020
e9edcfb
Merge branch 'master' into cat-from-int-codes
Feb 11, 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
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.0.2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Fixed regressions
Bug fixes
~~~~~~~~~

-
- Fixed bug where :meth:`Categorical.from_codes` improperly raised a ``ValueError`` when passed nullable integer codes. (:issue:`31779`)
-

.. ---------------------------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion pandas/core/arrays/categorical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,11 @@ def from_codes(cls, codes, categories=None, ordered=None, dtype=None):
)
raise ValueError(msg)

codes = np.asarray(codes) # #21767
if is_extension_array_dtype(codes) and is_integer_dtype(codes):
Copy link
Member

Choose a reason for hiding this comment

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

Hmm does this universally work if we just call to_numpy? I suppose could copy where undesired but maybe better than if...else branching

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the if / else isn't too pretty but I think we have to worry if codes is a numpy array already or possibly a bare list. Ideally it'd be nice to just do pd.array(codes) instead of np.asarray(codes), but it looks like min and max haven't been implemented yet for IntegerArray so we get an error soon after.

Copy link
Member

Choose a reason for hiding this comment

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

In principle we could do a pd.array(codes).to_numpy("int64")
But a problem with that is that it will convert an already integer array of potentially lower bitsize than int64

Copy link
Member

Choose a reason for hiding this comment

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

Has numpy a concept of "any integer dtype"?
Because then we could also simply do codes = np.asarray(codes, dtype="any int")

Another option could be this (to avoid converting an already int ndarray to int64)::

if not isinstance(codes, np.ndarray):
    codes = np.asarray(codes, dtype="int64")

which seems simpler as what you have now

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option could be this (to avoid converting an already int ndarray to int64)::

if not isinstance(codes, np.ndarray):
    codes = np.asarray(codes, dtype="int64")

which seems simpler as what you have now

I think we want it to raise if codes is something like [0.0, 1.0, 2.0] but this would make it a valid input

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 we want it to raise if codes is something like [0.0, 1.0, 2.0] but this would make it a valid input

Ah, yes indeed...

(we should probably try to gather some of those situations we run into handling array-like input including nullable dtypes, to see if we can extract some useful helpers for those cases)

# Avoid the implicit conversion of Int to object
codes = codes.to_numpy(dtype=np.int64, na_value=np.nan)
Copy link
Member

Choose a reason for hiding this comment

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

Codes shouldn't contain missing values (and the na_value=np.nan will not work anyway for a int64 dtype)

else:
codes = np.asarray(codes) # #21767
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to add the issue number in code generally, unless you add a full expl (so here just remove i)

if len(codes) and not is_integer_dtype(codes):
raise ValueError("codes need to be array-like integers")

Expand Down
9 changes: 9 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 @@ -560,6 +560,15 @@ def test_from_codes_neither(self):
with pytest.raises(ValueError, match=msg):
Categorical.from_codes([0, 1])

def test_from_codes_with_nullable_int(self):
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 also test with a NaN in the codes and Int64 (should raise)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. The error message that ends up getting raised is maybe not the best: ValueError: cannot convert to '<class 'numpy.int64'>'-dtype NumPy array with missing values. Specify an appropriate 'na_value' for this dtype.. Would you recommend creating one specific to this method when we have NA values?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we should probably catch the error to raise a better one (because the error message now would be really confusing for users)

codes = pd.array([0, 1], dtype="Int64")
categories = ["a", "b"]

result = Categorical.from_codes(codes, categories=categories)
expected = Categorical.from_codes(codes.astype(int), categories=categories)

tm.assert_categorical_equal(result, expected)

@pytest.mark.parametrize("dtype", [None, "category"])
def test_from_inferred_categories(self, dtype):
cats = ["a", "b"]
Expand Down