BUG: preserve name in set_categories (#17509) #17517

Giftlin · 2017-09-13T19:12:53Z

closes Series losts its name after set_categories #17509
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

TomAugspurger · 2017-09-13T19:29:47Z

pandas/core/categorical.py

@@ -2068,7 +2068,7 @@ def _delegate_method(self, name, *args, **kwargs):
        method = getattr(self.categorical, name)
        res = method(*args, **kwargs)
        if res is not None:
-            return Series(res, index=self.index)
+            return Series(res, index=self.index, name=self.name)


Ah, I misread the earlier. self is the instance of CategoricalAccessor, not the Series like I expected. So this approach won't work.

I think if you modify

pandas/pandas/core/categorical.py

Line 2078 in f11bbf2

return CategoricalAccessor(data.values, data.index)

to get data.name. and then modify CategoricalAccessor.__init__ to accept a name. data may not have a name, so maybe getattr(data, 'name', None). Make sense?

so the signature should follow like we do in pandas.core.indexes.accessors, e.g. (data, index, name=None)

TomAugspurger · 2017-09-13T19:39:48Z

Also, if you could add new tests to ensure the name is passed through, and a release note in doc/source/whatsnew/v0.21.0.txt under bug fixes.

pep8speaks · 2017-09-13T23:06:13Z

Hello @Giftlin! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on September 18, 2017 at 11:36 Hours UTC

jreback

tests!

jreback · 2017-09-13T23:17:26Z

pandas/core/categorical.py

@@ -2068,7 +2068,7 @@ def _delegate_method(self, name, *args, **kwargs):
        method = getattr(self.categorical, name)
        res = method(*args, **kwargs)
        if res is not None:
-            return Series(res, index=self.index)
+            return Series(res, index=self.index, name=self.name)


so the signature should follow like we do in pandas.core.indexes.accessors, e.g. (data, index, name=None)

codecov · 2017-09-13T23:31:38Z

Codecov Report

Merging #17517 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17517      +/-   ##
==========================================
- Coverage   91.18%   91.16%   -0.02%     
==========================================
  Files         163      163              
  Lines       49543    49544       +1     
==========================================
- Hits        45177    45169       -8     
- Misses       4366     4375       +9

Flag	Coverage Δ
#multiple	`88.95% <100%> (ø)`	⬆️
#single	`40.21% <75%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/categorical.py	`95.52% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.77% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f11bbf2...b817366. Read the comment docs.

codecov · 2017-09-13T23:31:44Z

Codecov Report

Merging #17517 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17517      +/-   ##
==========================================
- Coverage   91.25%    91.2%   -0.05%     
==========================================
  Files         163      163              
  Lines       49606    49625      +19     
==========================================
- Hits        45266    45261       -5     
- Misses       4340     4364      +24

Flag	Coverage Δ
#multiple	`88.99% <100%> (-0.03%)`	⬇️
#single	`40.19% <75%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/categorical.py	`95.57% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.23% <0%> (-1.82%)`	⬇️
pandas/tseries/offsets.py	`97% <0%> (-0.18%)`	⬇️
pandas/core/frame.py	`97.77% <0%> (-0.1%)`	⬇️
pandas/core/indexes/interval.py	`93.57% <0%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.53% <0%> (ø)`	⬆️
pandas/core/api.py	`100% <0%> (ø)`	⬆️
pandas/core/resample.py	`96.17% <0%> (+0.01%)`	⬆️
pandas/plotting/_core.py	`82.73% <0%> (+0.03%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 328c7e1...ce78e6c. Read the comment docs.

jreback · 2017-09-14T10:05:56Z

contrib docs are here. pls add tests IN this PR. It should fail with the tests, then pass after the fix.

Patch 7

- Bug in preserving name in set_categories. (:issue:`17509`)

TomAugspurger · 2017-09-17T12:08:11Z

doc/source/whatsnew/v0.21.0.txt

@@ -566,6 +566,7 @@ Categorical
 - Bug in the categorical constructor with empty values and categories causing
  the ``.categories`` to be an empty ``Float64Index`` rather than an empty
  ``Index`` with object dtype (:issue:`17248`)
+- Bug in preserving name in ``set_categories``. (:issue:`17509`)


Could you make this more descriptive? Maybe

Bug in categorical operations on Series like `Series.cat.set_categories` not preserving the original Series' name (:issue:`17509`)

TomAugspurger · 2017-09-17T12:10:35Z

pandas/tests/test_categorical.py

@@ -57,6 +57,13 @@ def test_getitem_listlike(self):
        expected = c[np.array([100000]).astype(np.int64)].codes
        tm.assert_numpy_array_equal(result, expected)

+    def test_getname_category(self):
+        result = 'A'


result should be the result of the operation. So. result = s.cat.set_categories([1, 2, 3]).name. And then compare directly to the expected name. assert result == 'A'

TomAugspurger · 2017-09-17T12:11:25Z

pandas/tests/test_categorical.py

+        s = s.cat.set_categories([1, 2, 3])
+        expected = s.astype('category').name
+        tm.assert_almost_equal(result, expected)
+


Can you add additional tests for the other delegated methods that this should apply to? set_ordered, etc?

TomAugspurger · 2017-09-17T12:31:16Z

pandas/tests/test_categorical.py

+        expected = 'A'
+        s = pd.Series([1, 2, 3], name='A').astype('category')
+        s = s.cat.set_categories([1, 2, 3])
+        result = s.astype('category').name


This should be result = s.cat.set_categories([1, 2, 3]).name so that we're directly testing the behavior that caused the bug.

TomAugspurger · 2017-09-17T12:31:55Z

doc/source/whatsnew/v0.21.0.txt

@@ -566,7 +566,9 @@ Categorical
 - Bug in the categorical constructor with empty values and categories causing
  the ``.categories`` to be an empty ``Float64Index`` rather than an empty
  ``Index`` with object dtype (:issue:`17248`)
-
+- Bug in categorical operations on Series like `Series.cat.set_categories`


Sorry, it should be double backticks, not single

``Series.cat.set_categories``

jreback · 2017-09-17T13:02:51Z

pandas/tests/test_categorical.py

@@ -57,6 +57,12 @@ def test_getitem_listlike(self):
        expected = c[np.array([100000]).astype(np.int64)].codes
        tm.assert_numpy_array_equal(result, expected)

+    def test_getname_category(self):


parametrize with all accessor methods

you can use a lambda expression

@pytest.mark.parametrize("method", [ lambda x: x.cat.set_categories([1, 2,3 ]), lambda x: x.cat.reorder_categories([2,3, 1], ordered=True), ]) def test_getname_categorical_accessor(self, method): s = .... expected = 'A' result = method(s).name assert result == expected

The current test function is also fine only right? What is the difference? Performance?

jreback · 2017-09-17T14:14:57Z

doc/source/whatsnew/v0.21.0.txt

@@ -566,7 +566,9 @@ Categorical
 - Bug in the categorical constructor with empty values and categories causing
  the ``.categories`` to be an empty ``Float64Index`` rather than an empty
  ``Index`` with object dtype (:issue:`17248`)
-
+- Bug in categorical operations on Series like ``Series.cat.set_categories``


should instead refer to all Series.cat operations not preserving name

Should I mention all the functions along with set_categories here?

I would just refer to http://pandas.pydata.org/pandas-docs/stable/categorical.html#working-with-categories

Lint error fix

Sorry for too many commits.

Lint error

jreback · 2017-09-17T20:55:10Z

pandas/tests/test_categorical.py

@@ -57,6 +57,24 @@ def test_getitem_listlike(self):
        expected = c[np.array([100000]).astype(np.int64)].codes
        tm.assert_numpy_array_equal(result, expected)

+    @pytest.mark.parametrize("method",


not sure if this is passing linting I would write this like

@pytest.mark.parametrize( "method", [ lambda ..... ])

to get more indent

jreback · 2017-09-17T20:55:56Z

ok this looks fine to me (after fixing linting). ping on green.

jorisvandenbossche

Looks good!
Just a minor comment about how to link to a section in the docs.

jorisvandenbossche · 2017-09-18T07:12:13Z

doc/source/whatsnew/v0.21.0.txt

@@ -566,6 +566,7 @@ Categorical
 - Bug in the categorical constructor with empty values and categories causing
  the ``.categories`` to be an empty ``Float64Index`` rather than an empty
  ``Index`` with object dtype (:issue:`17248`)
+- Bug in categorical operations `Series.cat <http://pandas.pydata.org/pandas-docs/stable/categorical.html#working-with-categories>' not preserving the original Series' name (:issue:`17509`)


Sphinx has the ability to do local references (see http://www.sphinx-doc.org/en/stable/markup/inline.html#cross-referencing-arbitrary-locations), so you can do :ref:`Series.cat <categorical.cat>` instead of listing the actual full url.
You only need to add a label at the specific location (put .. _categorical.cat: on the line before the title):

pandas/doc/source/categorical.rst

Lines 148 to 150 in cbb090f

Working with categories

-----------------------

This is the expected change right?

http://pandas.pydata.org/pandas-docs/stable/categorical.html#working-with-categories

Update categorical.rst

What's new

gfyoung · 2017-09-18T08:10:31Z

pandas/tests/test_categorical.py

+        s = pd.Series([1, 2, 3], name='A').astype('category')
+        expected = 'A'
+        result = method(s).name
+        assert result == expected


Instead of doing this, construct the expected Series directly from the constructor and call tm.assert_series_equal.

But Series is not expected. Only the name is expected, which is a character

That's because you defined your test as such. The output of method is a Series, not a character. I'm saying that you should check the output of method instead of comparing its name attribute.

We want to make sure that nothing else changes and that the name parameter is preserved.

@gfyoung In this case asserting only the name is fine I think. Assuming the actual methods are already tested separately, this tests just asserts for all of them they preserve the name. If checking the actual resulting series, all of them would need a different result, so would just over-complicate the test.

Update test_categorical.py

Giftlin · 2017-09-18T12:28:57Z

@jreback
@TomAugspurger
checks have passed

jorisvandenbossche · 2017-09-18T12:31:15Z

@Giftlin Thanks!

(I added a small commit with some white-space clean-up. Not sure what went wrong, but the diff showed changes in whitespace on some lines. You might need to check the settings of your editor to ensure it only uses spaces for whitespace)

…7517)

FIX for set_categories issue pandas-dev#17509

42b1c7d

TomAugspurger reviewed Sep 13, 2017

View reviewed changes

Giftlin added 2 commits September 14, 2017 04:32

FIX for set_categories issue pandas-dev#17509

3843746

FIX for set_categories issue pandas-dev#17509

4f731cf

Update categorical.py

a4ba634

jreback requested changes Sep 13, 2017

View reviewed changes

jreback added the Categorical Categorical Data Type label Sep 13, 2017

Update categorical.py

b817366

Giftlin mentioned this pull request Sep 14, 2017

test for #17517 category name None after set_category() change #17529

Closed

4 tasks

jorisvandenbossche changed the title ~~FIX for set_categories issue #17509~~ BUG: preserve name in set_categories (#17509) Sep 15, 2017

Giftlin added 5 commits September 15, 2017 21:05

Test for the changes

aa782dd

Merge pull request #2 from Giftlin/patch-7

70089a4

Patch 7

Space removed

4c30c96

- Bug in preserving name in set_categories. (:issue:17509)

42afe67

Merge pull request #3 from Giftlin/patch-10

73a6713

- Bug in preserving name in set_categories. (:issue:`17509`)

TomAugspurger requested changes Sep 17, 2017

View reviewed changes

Giftlin added 2 commits September 17, 2017 17:48

Update v0.21.0.txt

8f888ef

Update test_categorical.py

f09c9d5

TomAugspurger reviewed Sep 17, 2017

View reviewed changes

Giftlin added 2 commits September 17, 2017 18:25

Update v0.21.0.txt

8781d64

Update test_categorical.py

2ed3740

jreback requested changes Sep 17, 2017

View reviewed changes

Update test_categorical.py

8192515

jreback requested changes Sep 17, 2017

View reviewed changes

Giftlin added 5 commits September 18, 2017 00:28

Lint error fix

3b40433

Merge pull request #5 from Giftlin/Giftlin-lint-error-patch

b9ab8d3

Lint error fix

Removed space

e21fe08

Sorry for too many commits.

Lint error

9730757

Merge pull request #6 from Giftlin/Giftlinlint

1749a1c

Lint error

jreback reviewed Sep 17, 2017

View reviewed changes

jreback added the Bug label Sep 17, 2017

Giftlin added 3 commits September 18, 2017 07:23

Update test_categorical.py

5ae2453

Update test_categorical.py

9f25ad9

Update test_categorical.py

6b4f0f2

jorisvandenbossche added this to the 0.21.0 milestone Sep 18, 2017

jorisvandenbossche reviewed Sep 18, 2017

View reviewed changes

Giftlin added 5 commits September 18, 2017 12:54

Update v0.21.0.txt

68a2d86

http://pandas.pydata.org/pandas-docs/stable/categorical.html#working-with-categories

Update v0.21.0.txt

a94d52a

Update categorical.rst

73f5241

Merge pull request #9 from Giftlin/Giftlin-patch-1-1

f6e8aa9

Update categorical.rst

Merge pull request #7 from Giftlin/Giftlin-whatsnew

d4bbe27

What's new

gfyoung reviewed Sep 18, 2017

View reviewed changes

Giftlin added 2 commits September 18, 2017 13:50

Update test_categorical.py

7bb3378

Merge pull request #10 from Giftlin/Giftlin-test_categorical.py-1

7f5ec9b

Update test_categorical.py

jreback mentioned this pull request Sep 18, 2017

Series losts its name after set_categories #17509

Closed

TomAugspurger approved these changes Sep 18, 2017

View reviewed changes

small edits

ce78e6c

jorisvandenbossche merged commit 9cc3333 into pandas-dev:master Sep 18, 2017

Giftlin deleted the patch-4 branch September 18, 2017 18:01

alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

BUG: preserve name in set_categories (pandas-dev#17509) (pandas-dev#1…

002256a

…7517)

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

BUG: preserve name in set_categories (pandas-dev#17509) (pandas-dev#1…

cdada6b

…7517)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: preserve name in set_categories (#17509) #17517

BUG: preserve name in set_categories (#17509) #17517

Giftlin commented Sep 13, 2017 •

edited

Loading

TomAugspurger Sep 13, 2017

TomAugspurger Sep 13, 2017 •

edited

Loading

jreback Sep 13, 2017

TomAugspurger commented Sep 13, 2017

pep8speaks commented Sep 13, 2017 •

edited

Loading

jreback left a comment

jreback Sep 13, 2017

codecov bot commented Sep 13, 2017

codecov bot commented Sep 13, 2017 •

edited

Loading

jreback commented Sep 14, 2017

TomAugspurger Sep 17, 2017

TomAugspurger Sep 17, 2017

TomAugspurger Sep 17, 2017

TomAugspurger Sep 17, 2017 •

edited

Loading

TomAugspurger Sep 17, 2017

jreback Sep 17, 2017

jreback Sep 17, 2017

Giftlin Sep 17, 2017

jreback Sep 17, 2017

Giftlin Sep 17, 2017

jreback Sep 17, 2017

jreback Sep 17, 2017

jreback commented Sep 17, 2017

jorisvandenbossche left a comment

jorisvandenbossche Sep 18, 2017

Giftlin Sep 18, 2017

jorisvandenbossche Sep 18, 2017

gfyoung Sep 18, 2017

Giftlin Sep 18, 2017

gfyoung Sep 18, 2017 •

edited

Loading

jorisvandenbossche Sep 18, 2017

Giftlin commented Sep 18, 2017

jorisvandenbossche commented Sep 18, 2017

BUG: preserve name in set_categories (#17509) #17517

BUG: preserve name in set_categories (#17509) #17517

Conversation

Giftlin commented Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

TomAugspurger Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Sep 13, 2017

pep8speaks commented Sep 13, 2017 • edited Loading

Comment last updated on September 18, 2017 at 11:36 Hours UTC

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 13, 2017

Codecov Report

codecov bot commented Sep 13, 2017 • edited Loading

Codecov Report

jreback commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 17, 2017

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Giftlin commented Sep 18, 2017

jorisvandenbossche commented Sep 18, 2017

Giftlin commented Sep 13, 2017 •

edited

Loading

TomAugspurger Sep 13, 2017 •

edited

Loading

pep8speaks commented Sep 13, 2017 •

edited

Loading

codecov bot commented Sep 13, 2017 •

edited

Loading

TomAugspurger Sep 17, 2017 •

edited

Loading

gfyoung Sep 18, 2017 •

edited

Loading