BUG: BooleanArray.value_counts dropna #30824
Conversation
TomAugspurger
added
the
ExtensionArray
|
LGTM pending green |
| @@ -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(): | |||
There was a problem hiding this comment.
is the result an object dtyped Series when dropna=True? (add a test as well)
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm, OK, will update these.
There was a problem hiding this comment.
And so API breaking change for IntegerARrray.value_counts to return a nullalble int dtype too?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Not a hill I want to die on.
|
OK, updated {String,Integer,Boolean}Array.value_counts to return a nullable int64 dtype. I moved BooleanArray.value_counts and IntegerArray.value_counts to the base class. |
doc/source/whatsnew/v1.0.0.rst
Outdated
| .. ipython:: python | ||
|
|
||
| >>> pd.Series([2, 1, 1, None], dtype="Int64").value_counts().dtype | ||
| Int64Dtype() |
There was a problem hiding this comment.
Don't show the output (or >>> prompt), or otherwise also make it a code-block
There was a problem hiding this comment.
Whoops, thanks.
|
let me clarify my comments i think the index should be nullable ints as that preserves the intent here |
|
Thanks for clarifying. The index can't be nullable yet. @jorisvandenbossche thoughts on the values dtype? |
| @@ -411,6 +411,24 @@ 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** | |||
|
|
|||
There was a problem hiding this comment.
actually, even though is a breaking change, it puts nullable in more general usage. so actually i think this is a good change.
IMO, ideally we retain the nullable dtype in both the index and the values. But indeed, for index we can't do that yet, so for now it's only the values.
We are still returning an int dtype. But in the "nullable dtype universe"-subsystem, the "int64" dtype is the nullable int64 dtype. |
|
So I think that's some +1s and a -0 (#30824 (comment)). Merging in an hour or so unless @jbrockmendel objects. |
|
No objection here. |
|
Thanks. |
Closes #30685