Skip to content

TST: add tests for take() on empty arrays #20582

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 9 commits into from
Apr 17, 2018
Merged

TST: add tests for take() on empty arrays #20582

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4d7cc21
BUG: fix take() on empty arrays
jorisvandenbossche Apr 2, 2018
aba95d5
simplify check in categorical
jorisvandenbossche Apr 2, 2018
3d6fd2e
add docs
jorisvandenbossche Apr 2, 2018
eacb3d5
simplify check
jorisvandenbossche Apr 9, 2018
ee66d61
Merge remote-tracking branch 'upstream/master' into test-ea-empty-take
jorisvandenbossche Apr 12, 2018
c4faf8e
expand take test + undo changes in categorical
jorisvandenbossche Apr 12, 2018
9257203
add reindex test
jorisvandenbossche Apr 16, 2018
a3f88ee
add failing test for Series.take
jorisvandenbossche Apr 16, 2018
b5d357f
Merge remote-tracking branch 'upstream/master' into test-ea-empty-take
jorisvandenbossche Apr 16, 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
11 changes: 11 additions & 0 deletions pandas/core/arrays/categorical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1740,6 +1740,17 @@ def take_nd(self, indexer, allow_fill=True, fill_value=None):
# but is passed thru internally
assert isna(fill_value)

indexer = np.asarray(indexer)

# take on empty array
if not len(self):
# only valid if result is an all-missing array
if (indexer == -1).all():
codes = [-1] * len(indexer)
else:
raise IndexError(
"cannot do a non-empty take from an empty array.")

codes = take_1d(self._codes, indexer, allow_fill=True, fill_value=-1)
Copy link
Member Author

Choose a reason for hiding this comment

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

For the internal use cases (Categorical at the moment, later maybe more arrays), I could also push this test to take_1d. But, that is also used by many other things, so not sure I can just do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be here rather in take1d (i see you comment)

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 simplified the check a bit here (as it only needed to check the empty and not-all -1 case, as when indexer is all -1 take_1d works as expected on empty arrays).

@jreback I am not sure if I should move it to take_1d. It depends on the guarantees we want to give (ourselves) for take_1d (if it should do such bounds checking). It might be that currently all codes that uses take_1d/nd already does this bounds checking (I did not find a practical use case (apart from Categorical.take itself) where we run into this bug), and then doing another one in take_nd would give a performance penalty.

result = self._constructor(codes, categories=self.categories,
ordered=self.ordered, fastpath=True)
Expand Down
9 changes: 9 additions & 0 deletions pandas/tests/extension/base/getitem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import numpy as np

import pandas as pd
import pandas.util.testing as tm

from .base import BaseExtensionTests

Expand Down Expand Up @@ -120,3 +121,11 @@ def test_take_sequence(self, data):
assert result.iloc[0] == data[0]
assert result.iloc[1] == data[1]
assert result.iloc[2] == data[3]

def test_take_empty_missing(self, data, na_value, na_cmp):
empty = data[:0]
result = empty.take([-1])
na_cmp(result[0], na_value)

with tm.assert_raises_regex(IndexError, "cannot do a non-empty take"):
empty.take([0, 1])
Copy link
Member Author

Choose a reason for hiding this comment

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

@TomAugspurger I didn't find any existing tests for take, so not sure the indexing tests is the best place (maybe rather the BaseMethodsTests).
And, I could also add tests for the actual use cases where you get this (eg reindex on an empty series)

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like the right place for testing a behavior specific to take.

12 changes: 11 additions & 1 deletion pandas/tests/extension/decimal/array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def __len__(self):
return len(self.values)

def __repr__(self):
return repr(self.values)
return "DecimalArray: " + repr(self.values)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, meant to add this a while ago :)


@property
def nbytes(self):
Expand All @@ -75,9 +75,19 @@ def isna(self):
return np.array([x.is_nan() for x in self.values])

def take(self, indexer, allow_fill=True, fill_value=None):

indexer = np.asarray(indexer)
mask = indexer == -1

# take on empty array
if not len(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

DecimalArray.take does not use take_1d, so it's not possible to move it

Copy link
Contributor

Choose a reason for hiding this comment

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

then you need to ask yourself if this is the correct implementation. pandas implementst take_1d for exactly this reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

take_1d is not a public function. Do you want to expose it publicly?

# only valid if result is an all-missing array
if mask.all():
Copy link
Contributor

Choose a reason for hiding this comment

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

writing take is a fair amount of effort on the extension writer. meaning additional complexity and have to handle edge cases. take is a pure indexing operation and so ideally should be implemetned as high up in the stack as possible (meaning in the base class for EA) if the EA writer wants to override then ok, but it should just work generally. All of the machinery is already there. The point of the EA is to make it easy, having to handle all of these edge cases is not easy.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of the machinery is already there.

Assuming you have an ndarray. Do we want to provide a default implementation that does an astype(object) to convert to an ndarray of scalars, take on that, and then construct a new EA?

I'm going to make a PR documenting which methods use .astype(object), so that the EA author should consider overriding them if performance is a concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming you have an ndarray. Do we want to provide a default implementation that does an astype(object) to convert to an ndarray of scalars, take on that, and then construct a new EA?

It will almost never be what you want I think. It could still serve as an example implementation of course, like we now have one in the docstring.

return type(self)([self._na_value] * len(indexer))
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

no else needed. is the raise hit?

raise IndexError(
"cannot do a non-empty take from an empty array.")

indexer = _ensure_platform_int(indexer)
out = self.values.take(indexer)
out[mask] = self._na_value
Expand Down
7 changes: 5 additions & 2 deletions pandas/tests/extension/json/array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,11 @@ def isna(self):
return np.array([x == self._na_value for x in self.data])

def take(self, indexer, allow_fill=True, fill_value=None):
output = [self.data[loc] if loc != -1 else self._na_value
for loc in indexer]
try:
output = [self.data[loc] if loc != -1 else self._na_value
for loc in indexer]
except IndexError:
raise IndexError("cannot do a non-empty take from an empty array.")
return self._constructor_from_sequence(output)

def copy(self, deep=False):
Expand Down