BUG: value_counts not working correctly on ExtensionArrays #33674

kotamatsuoka · 2020-04-20T13:10:13Z

closes value_counts not working correctly on (some?) ExtensionArrays #33172
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pandas/core/algorithms.py

jreback

pls add a whatsnew note as well, you can put it in numeric bug fix section

pandas/tests/arrays/string_/test_string.py

simonjayhawkins · 2020-04-21T09:45:56Z

This could be an opportunity to use the more performant sum function from core\array_algos\masked_reductions.py and add a sum method to IntegerArray (or maybe BaseMaskedArray) There is a precedent for adding sum methods to extension arrays (timedeltas and sparse) but not sure how this fits in with the EA interface. @jorisvandenbossche?

pep8speaks · 2020-04-21T13:54:36Z

Hello @kotamatsuoka! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-30 23:45:49 UTC

dsaxton · 2020-04-23T18:55:54Z

This could be an opportunity to use the more performant sum function from core\array_algos\masked_reductions.py and add a sum method to IntegerArray (or maybe BaseMaskedArray) There is a precedent for adding sum methods to extension arrays (timedeltas and sparse) but not sure how this fits in with the EA interface. @jorisvandenbossche?

@simonjayhawkins That's the approach I was taking here although am not sure at the moment how best to implement this: #33538

simonjayhawkins · 2020-04-23T19:15:37Z

hmm, didn't realise that this PR overlaps with #33538.

simonjayhawkins

Thanks @kotamatsuoka for working on this.

the bug is fixed on master, see #33172 (comment)

just needs tests.

dsaxton · 2020-04-26T12:46:59Z

pandas/core/arrays/string_.py

@@ -290,7 +290,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).astype("Int64")
+        return value_counts(self._ndarray, dropna=dropna)


Can you revert this back to what it was on master (will need to change the string tests back as well)?

If revert, value_counts method return different dtype and need to change many tests. (comment)

@dsaxton why/where were these changed to Int64 in the first place?

It looks like it was added here #30824

@jreback this is converted to Int64 on purpose, in the idea that operations on nullable columns should, to the extent possible, also result in new nullable dtypes (similarly as doing a comparison operation with string dtype will give a nullable boolean dtype)

pandas/tests/extension/base/methods.py

jreback · 2020-04-26T20:01:21Z

pandas/core/arrays/string_.py

@@ -290,7 +290,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).astype("Int64")
+        return value_counts(self._ndarray, dropna=dropna)


@dsaxton why/where were these changed to Int64 in the first place?

jreback · 2020-04-26T20:02:43Z

this also needs a whatsnew note, add in bug fixes, extension array section in 1.1

jbrockmendel · 2020-04-27T14:30:30Z

pandas/tests/extension/test_numpy.py

@@ -199,6 +199,10 @@ class TestMethods(BaseNumPyTests, base.BaseMethodsTests):
    def test_value_counts(self, all_data, dropna):
        pass

+    @pytest.mark.skip(reason="value_counts not implemented")


should be skip or xfail?

pandas/tests/extension/test_integer.py

pandas/tests/extension/test_sparse.py

jorisvandenbossche · 2020-04-29T12:03:10Z

@kotamatsuoka the base test that you added is now being skipped for several ExtensionArray classes (which is fine, to be clear), but I think that we should then test it specifically for at least some of those classes with a custom test (I would at least test it for IntegerArray and BooleanArray).

So instead of xfailing or skipping the test in test_boolean.py and test_integer.py, I would actually have a custom version of the test there (that has a different expected value, to match the behaviour).

(or is this already tested elsewhere?)

kotamatsuoka · 2020-04-29T12:41:05Z

Thanks @jorisvandenbossche .
I updated test_value_counts_with_normalize(), so xfailing or skipping the test in test_boolean.py , test_integer.py and test_sparse.py are no longer needed。

pandas/tests/extension/test_numpy.py

pandas/tests/extension/base/methods.py

pandas/tests/extension/test_numpy.py

jreback

lgtm. ping on green.

jreback · 2020-05-01T00:34:48Z

thanks @kotamatsuoka

…v#33674)

BUG: value_counts not working correctly on ExtensionArrays

11c26e2

simonjayhawkins reviewed Apr 20, 2020

View reviewed changes

pandas/core/algorithms.py Outdated Show resolved Hide resolved

simonjayhawkins added Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Apr 20, 2020

kotamatsuoka force-pushed the normalize-value-counts branch from 949ef4f to ea5f7c4 Compare April 20, 2020 14:56

jbrockmendel reviewed Apr 20, 2020

View reviewed changes

pandas/core/algorithms.py Outdated Show resolved Hide resolved

Change sum() to ndarray.sum()

b025232

kotamatsuoka force-pushed the normalize-value-counts branch from ea5f7c4 to b025232 Compare April 20, 2020 15:35

jreback requested changes Apr 20, 2020

View reviewed changes

pandas/tests/arrays/string_/test_string.py Outdated Show resolved Hide resolved

jreback added this to the 1.1 milestone Apr 21, 2020

kotamatsuoka force-pushed the normalize-value-counts branch from c6c4751 to 67c83e9 Compare April 21, 2020 13:57

kotamatsuoka requested a review from jreback April 21, 2020 13:58

kotamatsuoka force-pushed the normalize-value-counts branch 2 times, most recently from 957d105 to 4235020 Compare April 21, 2020 16:32

Expand tests

7007205

kotamatsuoka force-pushed the normalize-value-counts branch from 4235020 to 7007205 Compare April 22, 2020 14:07

Update test

af97126

kotamatsuoka added 2 commits April 24, 2020 12:56

Resolve warning

cbabcad

Pass test in Windows py36_np15

dac1af5

kotamatsuoka force-pushed the normalize-value-counts branch from 2d857d1 to dac1af5 Compare April 24, 2020 21:53

kotamatsuoka added 2 commits April 25, 2020 13:36

Change Int32 to Int64

0be33fc

Merge branch 'master' into normalize-value-counts

660bba3

simonjayhawkins requested changes Apr 25, 2020

View reviewed changes

simonjayhawkins added Testing pandas testing functions or related to the test suite and removed Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Apr 25, 2020

kotamatsuoka requested a review from jreback April 26, 2020 06:42

dsaxton reviewed Apr 26, 2020

View reviewed changes

jreback requested changes Apr 26, 2020

View reviewed changes

kotamatsuoka force-pushed the normalize-value-counts branch from 4e283ff to badbaa4 Compare April 27, 2020 13:59

jbrockmendel reviewed Apr 27, 2020

View reviewed changes

pandas/tests/extension/test_integer.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Apr 27, 2020

View reviewed changes

pandas/tests/extension/test_sparse.py Outdated Show resolved Hide resolved

kotamatsuoka force-pushed the normalize-value-counts branch from badbaa4 to bf91882 Compare April 27, 2020 15:16

split new test

67ee980

kotamatsuoka force-pushed the normalize-value-counts branch from bf91882 to 67ee980 Compare April 28, 2020 11:20

kotamatsuoka requested a review from jbrockmendel April 28, 2020 21:25

Update normalize value_counts test

405037f

jreback reviewed Apr 29, 2020

View reviewed changes

pandas/tests/extension/test_numpy.py Outdated Show resolved Hide resolved

jorisvandenbossche approved these changes Apr 30, 2020

View reviewed changes

jreback requested changes Apr 30, 2020

View reviewed changes

pandas/tests/extension/base/methods.py Show resolved Hide resolved

pandas/tests/extension/test_numpy.py Outdated Show resolved Hide resolved

jreback approved these changes Apr 30, 2020

View reviewed changes

kotamatsuoka force-pushed the normalize-value-counts branch 5 times, most recently from cdc2f09 to 3473a42 Compare April 30, 2020 23:11

add coments

5d54a05

kotamatsuoka force-pushed the normalize-value-counts branch from 3473a42 to 5d54a05 Compare April 30, 2020 23:45

jreback merged commit c00293d into pandas-dev:master May 1, 2020

kotamatsuoka deleted the normalize-value-counts branch May 1, 2020 00:58

rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020

BUG: value_counts not working correctly on ExtensionArrays (pandas-de…

5a8dbf7

…v#33674)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: value_counts not working correctly on ExtensionArrays #33674

BUG: value_counts not working correctly on ExtensionArrays #33674

kotamatsuoka commented Apr 20, 2020 •

edited

Loading

jreback left a comment

simonjayhawkins commented Apr 21, 2020

pep8speaks commented Apr 21, 2020 •

edited

Loading

dsaxton commented Apr 23, 2020

simonjayhawkins commented Apr 23, 2020

simonjayhawkins left a comment

dsaxton Apr 26, 2020

kotamatsuoka Apr 26, 2020

jreback Apr 26, 2020

dsaxton Apr 26, 2020

jorisvandenbossche Apr 27, 2020

jreback Apr 26, 2020

jreback commented Apr 26, 2020

jbrockmendel Apr 27, 2020

jorisvandenbossche commented Apr 29, 2020

kotamatsuoka commented Apr 29, 2020

jreback left a comment

jreback commented May 1, 2020

BUG: value_counts not working correctly on ExtensionArrays #33674

BUG: value_counts not working correctly on ExtensionArrays #33674

Conversation

kotamatsuoka commented Apr 20, 2020 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

simonjayhawkins commented Apr 21, 2020

pep8speaks commented Apr 21, 2020 • edited Loading

Comment last updated at 2020-04-30 23:45:49 UTC

dsaxton commented Apr 23, 2020

simonjayhawkins commented Apr 23, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

dsaxton Apr 26, 2020

Choose a reason for hiding this comment

kotamatsuoka Apr 26, 2020

Choose a reason for hiding this comment

jreback Apr 26, 2020

Choose a reason for hiding this comment

dsaxton Apr 26, 2020

Choose a reason for hiding this comment

jorisvandenbossche Apr 27, 2020

Choose a reason for hiding this comment

jreback Apr 26, 2020

Choose a reason for hiding this comment

jreback commented Apr 26, 2020

jbrockmendel Apr 27, 2020

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 29, 2020

kotamatsuoka commented Apr 29, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback commented May 1, 2020

kotamatsuoka commented Apr 20, 2020 •

edited

Loading

pep8speaks commented Apr 21, 2020 •

edited

Loading