Skip to content

BUG: BooleanArray.value_counts dropna #30824

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 5 commits into from
Jan 9, 2020
Merged

BUG: BooleanArray.value_counts dropna #30824

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d1abac
BUG: BooleanArray.value_counts dropna
TomAugspurger Jan 8, 2020
b3796a5
Merge remote-tracking branch 'upstream/master' into 30685-value_counts
TomAugspurger Jan 9, 2020
604f170
fixup
TomAugspurger Jan 9, 2020
12cfbeb
remove unused numpy_dtype
TomAugspurger Jan 9, 2020
67c0c06
doc fixup
TomAugspurger Jan 9, 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
3 changes: 2 additions & 1 deletion pandas/core/arrays/boolean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,8 @@ def value_counts(self, dropna=True):
# w/o passing the dtype
array = np.append(array, [self._mask.sum()])
index = Index(
np.concatenate([index, np.array([np.nan], dtype=object)]), dtype=object
np.concatenate([index, np.array([self.dtype.na_value], dtype=object)]),
dtype=object,
)

return Series(array, index=index)
Expand Down
7 changes: 7 additions & 0 deletions pandas/tests/arrays/test_boolean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,3 +856,10 @@ def test_arrow_roundtrip():
result = table.to_pandas()
assert isinstance(result["a"].dtype, pd.BooleanDtype)
tm.assert_frame_equal(result, df)


def test_value_counts_na():
Copy link
Contributor

Choose a reason for hiding this comment

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

is the result an object dtyped Series when dropna=True? (add a test as well)

Copy link
Member

Choose a reason for hiding this comment

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

In neither cases (dropna True of False) is the result an object dtype series, it is always integer (it are just counts).

That said, should the result here rather be a nullable integer type? Not that there are nulls here, but in the light of "trying to return nullable types as much as possible from operations involving nullable types".

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, yeah i think we should just move this to return a nullable integer (as this is new api). will promote consistency in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, OK, will update these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And so API breaking change for IntegerARrray.value_counts to return a nullalble int dtype too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you expand on why you find it weird?

It's true that the result of a value_counts will always have no NAs, but returning a nullable int type prevents a reintroduction of NAs in subsequent operations from converting to float.

Copy link
Member

Choose a reason for hiding this comment

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

Can you expand on why you find it weird?

The motivation is to maintain consistency of "operations with nullable types return nullable types". But making value_counts().values return IntNA breaks the consistency of "values_counts().values is always np.int64". So it's a wash on "maintaining consistency".

Ideally we'd retain the dtype in the value_counts().index, and it seems like we're saying here "well we cant do that, so let's shoehorn the dtype into the values"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems like we're saying here "well we cant do that, so let's shoehorn the dtype into the values"

No, I don't think we're saying that. I think we're saying we find a nullable integer dtype to be more useful.

Copy link
Member

Choose a reason for hiding this comment

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

Not a hill I want to die on.

Copy link
Contributor

Choose a reason for hiding this comment

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

commented below

arr = pd.array([True, False, pd.NA], dtype="boolean")
result = arr.value_counts(dropna=False)
expected = pd.Series([1, 1, 1], index=[True, False, pd.NA])
tm.assert_series_equal(result, expected)