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.

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
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
19 changes: 19 additions & 0 deletions doc/source/whatsnew/v1.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,25 @@ Use :meth:`arrays.IntegerArray.to_numpy` with an explicit ``na_value`` instead.

a.to_numpy(dtype="float", na_value=np.nan)

**value_counts returns a nullable integer dtype**

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, even though is a breaking change, it puts nullable in more general usage. so actually i think this is a good change.

:meth:`Series.value_counts` with a nullable integer dtype now returns a nullable
integer dtype for the values.

*pandas 0.25.x*

.. code-block:: python

>>> pd.Series([2, 1, 1, None], dtype="Int64").value_counts().dtype
dtype('int64')

*pandas 1.0.0*

.. ipython:: python

>>> pd.Series([2, 1, 1, None], dtype="Int64").value_counts().dtype
Int64Dtype()
Copy link
Member

Choose a reason for hiding this comment

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

Don't show the output (or >>> prompt), or otherwise also make it a code-block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, thanks.


See :ref:`missing_data.NA` for more on the differences between :attr:`pandas.NA`
and :attr:`numpy.nan`.

Expand Down
46 changes: 0 additions & 46 deletions pandas/core/arrays/boolean.py
Original file line number Diff line number Diff line change
Expand Up @@ -410,52 +410,6 @@ def astype(self, dtype, copy=True):
data = self.to_numpy(na_value=na_value)
return astype_nansafe(data, dtype, copy=False)

def value_counts(self, dropna=True):
"""
Returns a Series containing counts of each category.

Every category will have an entry, even those with a count of 0.

Parameters
----------
dropna : bool, default True
Don't include counts of NaN.

Returns
-------
counts : Series

See Also
--------
Series.value_counts

"""

from pandas import Index, Series

# compute counts on the data with no nans
data = self._data[~self._mask]
value_counts = Index(data).value_counts()
array = value_counts.values

# TODO(extension)
# if we have allow Index to hold an ExtensionArray
# this is easier
index = value_counts.index.values.astype(bool).astype(object)

# if we want nans, count the mask
if not dropna:

# TODO(extension)
# appending to an Index *always* infers
# 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
)

return Series(array, index=index)

def _values_for_argsort(self) -> np.ndarray:
"""
Return values for sorting.
Expand Down
49 changes: 0 additions & 49 deletions pandas/core/arrays/integer.py
Original file line number Diff line number Diff line change
Expand Up @@ -467,55 +467,6 @@ def _ndarray_values(self) -> np.ndarray:
"""
return self._data

def value_counts(self, dropna=True):
"""
Returns a Series containing counts of each category.

Every category will have an entry, even those with a count of 0.

Parameters
----------
dropna : bool, default True
Don't include counts of NaN.

Returns
-------
counts : Series

See Also
--------
Series.value_counts

"""

from pandas import Index, Series

# compute counts on the data with no nans
data = self._data[~self._mask]
value_counts = Index(data).value_counts()
array = value_counts.values

# TODO(extension)
# if we have allow Index to hold an ExtensionArray
# this is easier
index = value_counts.index.astype(object)

# if we want nans, count the mask
if not dropna:

# TODO(extension)
# appending to an Index *always* infers
# w/o passing the dtype
array = np.append(array, [self._mask.sum()])
index = Index(
np.concatenate(
[index.values, np.array([self.dtype.na_value], dtype=object)]
),
dtype=object,
)

return Series(array, index=index)

def _values_for_factorize(self) -> Tuple[np.ndarray, Any]:
# TODO: https://github.com/pandas-dev/pandas/issues/30037
# use masked algorithms, rather than object-dtype / np.nan.
Expand Down
47 changes: 47 additions & 0 deletions pandas/core/arrays/masked.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,50 @@ def copy(self):
data = data.copy()
mask = mask.copy()
return type(self)(data, mask, copy=False)

def value_counts(self, dropna=True):
"""
Returns a Series containing counts of each unique value.

Parameters
----------
dropna : bool, default True
Don't include counts of missing values.

Returns
-------
counts : Series

See Also
--------
Series.value_counts
"""
from pandas import Index, Series
from pandas.arrays import IntegerArray

# compute counts on the data with no nans
data = self._data[~self._mask]
value_counts = Index(data).value_counts()

# TODO(extension)
# if we have allow Index to hold an ExtensionArray
# this is easier
index = value_counts.index.values.astype(object)

# if we want nans, count the mask
if dropna:
counts = value_counts.values
else:
counts = np.empty(len(value_counts) + 1, dtype="int64")
counts[:-1] = value_counts
counts[-1] = self._mask.sum()

index = Index(
np.concatenate([index, np.array([self.dtype.na_value], dtype=object)]),
dtype=object,
)

mask = np.zeros(len(counts), dtype="bool")
counts = IntegerArray(counts, mask)

return Series(counts, index=index)
2 changes: 1 addition & 1 deletion pandas/core/arrays/string_.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ def _reduce(self, name, skipna=True, **kwargs):
def value_counts(self, dropna=False):
from pandas import value_counts

return value_counts(self._ndarray, dropna=dropna)
return value_counts(self._ndarray, dropna=dropna).astype("Int64")

# Overrride parent because we have different return types.
@classmethod
Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/arrays/string_/test_string.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,14 @@ def test_arrow_roundtrip():
tm.assert_frame_equal(result, df)
# ensure the missing value is represented by NA and not np.nan or None
assert result.loc[2, "a"] is pd.NA


def test_value_counts_na():
arr = pd.array(["a", "b", "a", pd.NA], dtype="string")
result = arr.value_counts(dropna=False)
expected = pd.Series([2, 1, 1], index=["a", "b", pd.NA], dtype="Int64")
tm.assert_series_equal(result, expected)

result = arr.value_counts(dropna=True)
expected = pd.Series([2, 1], index=["a", "b"], dtype="Int64")
tm.assert_series_equal(result, expected)
11 changes: 11 additions & 0 deletions pandas/tests/arrays/test_boolean.py
Original file line number Diff line number Diff line change
Expand Up @@ -856,3 +856,14 @@ 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], dtype="Int64")
tm.assert_series_equal(result, expected)

result = arr.value_counts(dropna=True)
expected = pd.Series([1, 1], index=[True, False], dtype="Int64")
tm.assert_series_equal(result, expected)
11 changes: 11 additions & 0 deletions pandas/tests/arrays/test_integer.py
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,17 @@ def test_stat_method(pandasmethname, kwargs):
assert expected == result


def test_value_counts_na():
arr = pd.array([1, 2, 1, pd.NA], dtype="Int64")
result = arr.value_counts(dropna=False)
expected = pd.Series([2, 1, 1], index=[1, 2, pd.NA], dtype="Int64")
tm.assert_series_equal(result, expected)

result = arr.value_counts(dropna=True)
expected = pd.Series([2, 1], index=[1, 2], dtype="Int64")
tm.assert_series_equal(result, expected)


# TODO(jreback) - these need testing / are broken

# shift
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/extension/test_boolean.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ def test_searchsorted(self, data_for_sorting, as_series):
sorter = np.array([1, 0])
assert data_for_sorting.searchsorted(a, sorter=sorter) == 0

@pytest.mark.skip(reason="uses nullable integer")
def test_value_counts(self, all_data, dropna):
return super().test_value_counts(all_data, dropna)


class TestCasting(base.BaseCastingTests):
pass
Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/extension/test_integer.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ class TestMissing(base.BaseMissingTests):


class TestMethods(base.BaseMethodsTests):
@pytest.mark.parametrize("dropna", [True, False])
@pytest.mark.skip(reason="uses nullable integer")
def test_value_counts(self, all_data, dropna):
all_data = all_data[:10]
if dropna:
Expand Down
4 changes: 3 additions & 1 deletion pandas/tests/extension/test_string.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ class TestNoReduce(base.BaseNoReduceTests):


class TestMethods(base.BaseMethodsTests):
pass
@pytest.mark.skip(reason="returns nullable")
def test_value_counts(self, all_data, dropna):
return super().test_value_counts(all_data, dropna)


class TestCasting(base.BaseCastingTests):
Expand Down