BUG: make tz_localize operate on values rather than categories #28300

MarcoGorelli · 2019-09-05T17:28:44Z

closes series.dt.tz_localize() on Categorical operates on categories, not values #27952
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jbrockmendel · 2019-09-05T20:58:09Z

pandas/core/indexes/accessors.py

@@ -324,7 +324,9 @@ def __new__(cls, data):

        orig = data if is_categorical_dtype(data) else None
        if orig is not None:
-            data = Series(orig.values.categories, name=orig.name, copy=False)
+            data = Series(
+                orig.values.astype("datetime64[ns]"), name=orig.name, copy=False


why the astype? what if we get here with e.g. timedelta64 or Period?

Agreed, it was just a quick attempt based on what I'd read in the issue (but without having fully understood the issue), sorry for having put this up so soon, will work on it further

No sweat. ping me when this is ready for review

@jbrockmendel thanks - I'm happy to have this reviewed, whenever you have time

jreback · 2019-09-07T17:32:03Z

pandas/core/indexes/accessors.py

@@ -324,8 +321,7 @@ def __new__(cls, data):

        orig = data if is_categorical_dtype(data) else None
        if orig is not None:
-            data = Series(orig.values.categories, name=orig.name, copy=False)
-
+            data = Series(orig.values.to_list(), name=orig.name, copy=False)


so we don't want to convert to a list, I think rather we should still operate on the categories
and just return a new Categorical

I'm not sure I understand - is data supposed to be a pandas.Categorical? If so, then it won't pass the

if not isinstance(data, ABCSeries): raise TypeError( "cannot convert an object of type {0} to a " "datetimelike index".format(type(data)) )

test when we get to

if is_datetime64_dtype(data.dtype): return DatetimeProperties(data, orig)

is data supposed to be a pandas.Categorical?

At this point data is a Series with CategoricalDtype.

I think @jreback's point is that we don't want to call to_list() because that is costly

jreback · 2019-09-07T17:32:56Z

pandas/tests/series/test_timezones.py

@@ -373,6 +373,17 @@ def test_tz_localize_convert_copy_inplace_mutate(self, copy, method, tz):
        )
        tm.assert_series_equal(result, expected)

+    def test_tz_localize_categorical(self, tz_aware_fixture):
+        # GH 27952


this should be in the test_categorical tests

jreback · 2019-09-07T17:34:07Z

doc/source/whatsnew/v1.0.0.rst

@@ -88,7 +88,7 @@ Categorical
 ^^^^^^^^^^^

 - Added test to assert the :func:`fillna` raises the correct ValueError message when the value isn't a value from categories (:issue:`13628`)
-
+- Bug in :meth:`CombinedDatetimelikeProperties.__new__` and :meth:`Properties._delegate_property_get` was resulting in properties being applied to categories rather than values (:issue:`27952`)


it is not clear to a user what you are actually doing, try to use a little more obvious language, like

using date accessors on a categorical dtyped series of datetimes was incorrectly returning a ....

jreback · 2019-09-07T17:35:25Z

this also needs a bit more general testing, try to add a few more accessor functions that you are testing.

MarcoGorelli · 2019-09-16T15:52:13Z

Thanks @jbrockmendel and @jreback for your comments.

I've moved the location of the tests to indexes/test_category and have removed the to_list call. However, I am using .__array__(). Is this also expensive?

I can't see how to keep data as a Series with CategoricalDtype, because then I'm blocked by #28448 .

jbrockmendel · 2019-09-16T19:48:34Z

pandas/core/indexes/accessors.py

@@ -324,7 +321,12 @@ def __new__(cls, data):

        orig = data if is_categorical_dtype(data) else None
        if orig is not None:
-            data = Series(orig.values.categories, name=orig.name, copy=False)
+            data = Series(
+                orig.values.__array__(),


dont use __array__() directly. this is meant to be used as np.asarray(orig.values)

jbrockmendel · 2019-09-16T19:49:48Z

pandas/tests/indexes/test_category.py

+        expected = datetimes.dt.tz_localize(tz)
+        tm.assert_series_equal(result, expected)
+
+    @pytest.mark.parametrize("accessor", [("year"), ("month"), ("day")])


parens around year/month/day look unnecessary

jbrockmendel · 2019-09-16T19:50:20Z

pandas/tests/indexes/test_category.py

+        categorical = datetimes.astype("category")
+        result = categorical.dt.tz_localize(tz)
+        expected = datetimes.dt.tz_localize(tz)
+        tm.assert_series_equal(result, expected)


can you do the same with tz_convert (starting from a tz-aware dtype)?

should the result here be datetime64tz or categorical[datetime64tz]? My initial intuition is that it would stay categorical

@jbrockmendel thanks for your feedback! I've made the changes you suggested.

Regarding the result dtype - the example from #27952 returns

dtype: datetime64[ns]

and so this isn't affected by my changes. Perhaps it should be a separate issue?

Also, the docs say

Note The returned Series (or DataFrame) is of the same type as if you used the .str. / .dt. on a Series of that type (and not of type category!).

so this is consistent with that - but I'm happy to work on changing this if that's what you prefer

jreback · 2019-09-18T12:41:43Z

doc/source/whatsnew/v1.0.0.rst

@@ -110,7 +110,7 @@ Categorical
 ^^^^^^^^^^^

 - Added test to assert the :func:`fillna` raises the correct ValueError message when the value isn't a value from categories (:issue:`13628`)
-
+- Using date accessors on a categorical dtyped series of datetimes was incorrectly operating only on the unique categories (:issue: `27952`)


mention Series.dt.tz_localize here as an example

jreback · 2019-09-18T12:44:23Z

pandas/tests/indexes/test_category.py

+        tm.assert_series_equal(result, expected)
+
+    def test_dt_tz_convert(self, tz_aware_fixture):
+        # GH 27952


i think these should go in pandas/tests/series/tests_datetime_values.py

TomAugspurger · 2019-09-23T16:28:16Z

doc/source/whatsnew/v1.0.0.rst

@@ -135,7 +135,9 @@ Categorical

 - Added test to assert the :func:`fillna` raises the correct ValueError message when the value isn't a value from categories (:issue:`13628`)
 - Bug in :meth:`Categorical.astype` where ``NaN`` values were handled incorrectly when casting to int (:issue:`28406`)
-
+- Using date accessors on a categorical dtyped series of datetimes was not returning a Series (or DataFrame) of the


I'm having a hard time following this. Is the "not returning a Series / DataFrame" the same issue as the "skipping duplicates?"

If they're distinct issue, break it into multiple lines.

@TomAugspurger the only issue is that it's acting on the unique categories rather than on the values. I've tried rephrasing this more clearly

TomAugspurger · 2019-09-24T14:56:58Z

doc/source/whatsnew/v1.0.0.rst

@@ -165,7 +165,9 @@ Categorical

 - Added test to assert the :func:`fillna` raises the correct ValueError message when the value isn't a value from categories (:issue:`13628`)
 - Bug in :meth:`Categorical.astype` where ``NaN`` values were handled incorrectly when casting to int (:issue:`28406`)
-
+- Using date accessors on a categorical dtyped :class:`Series` of datetimes was not returning the same object that would


These :meth: aren't correct, since they don't resolve to a real method.

How about

Suggested change

- Using date accessors on a categorical dtyped :class:`Series` of datetimes was not returning the same object that would

- Bug in :class:`Series.str` and :class:`Series.dt` accessor for categorical-dtype returning results with the incorrect length (:issue:`27952`)

Sure - I've rephrased the first part as you've suggested, but have kept the Series.dt.tz_localize example as suggested by @jreback

MarcoGorelli · 2019-10-08T16:45:53Z

@TomAugspurger @jbrockmendel @jreback thanks for your reviews. Now that #28762 has been closed, I've been able to push a solution to this which doesn't involve np.asarray and IMO is a marginally cleaner.

Is this something you're still interested in? Should the return type (and thus the documentation) be changed as well?

If not, no worries, thanks for the time you've already dedicated to helping me, feel free to close it :)

jreback · 2019-10-08T16:59:13Z

sure this still makes sense

jbrockmendel · 2019-11-02T03:10:41Z

@MarcoGorelli can you rebase and we'll take a fresh look and try to get this through

MarcoGorelli · 2019-11-02T09:49:54Z

Sure, done

jreback

lgtm. some stylistic commments, ping on green.

jreback · 2019-11-02T15:55:00Z

pandas/core/indexes/accessors.py

@@ -324,7 +321,12 @@ def __new__(cls, data):

        orig = data if is_categorical_dtype(data) else None
        if orig is not None:
-            data = Series(orig.values.categories, name=orig.name, copy=False)
+            data = Series(
+                orig.values,


make this orig.array (doesn't make a practical difference), but is more idiomatic to our usecase here.

jreback · 2019-11-02T15:55:45Z

pandas/tests/series/test_datetime_values.py

@@ -344,6 +344,39 @@ def test_dt_namespace_accessor_categorical(self):
        expected = Series([2017, 2017, 2018, 2018], name="foo")
        tm.assert_series_equal(result, expected)

+    def test_dt_tz_localize(self, tz_aware_fixture):


can you add categorical to these test names to indicate what is going onhere.

MarcoGorelli · 2019-11-02T20:10:39Z

@jreback Sure, here you go

jreback · 2019-11-02T20:19:41Z

thanks @MarcoGorelli nice patch!

…s-dev#28300)

jbrockmendel reviewed Sep 5, 2019

View reviewed changes

MarcoGorelli changed the title ~~WIP: make tz_localize operate on values rather than categories~~ BUG: make tz_localize operate on values rather than categories Sep 7, 2019

jreback requested changes Sep 7, 2019

View reviewed changes

jreback added Categorical Categorical Data Type Datetime Datetime data dtype labels Sep 7, 2019

MarcoGorelli force-pushed the tz-localize-categories branch from 03fd748 to 22002e6 Compare September 14, 2019 14:24

jbrockmendel reviewed Sep 16, 2019

View reviewed changes

jreback requested changes Sep 18, 2019

View reviewed changes

MarcoGorelli force-pushed the tz-localize-categories branch from e5b8208 to e5545a7 Compare September 20, 2019 08:34

TomAugspurger reviewed Sep 23, 2019

View reviewed changes

TomAugspurger reviewed Sep 24, 2019

View reviewed changes

MarcoGorelli force-pushed the tz-localize-categories branch from 5d903a0 to 4b0c2a8 Compare October 8, 2019 16:03

MarcoGorelli and others added 9 commits November 2, 2019 09:45

Operate on categories

8d8df82

remove unused local variable

98b92cc

Use __array__ instead of to_list, set dtype in Series

1c15441

Add tz_convert test

44d22a7

Rephrase whatsnew entry, move tests

3126408

Remove duplicated tests

c15b457

Rephrase whatnew entry

3ac3e73

Fix conflict

dc9ed3d

Remove np.asarray, as pandas-dev#28762 has been fixed

4d0320c

MarcoGorelli force-pushed the tz-localize-categories branch from 4b0c2a8 to 4d0320c Compare November 2, 2019 09:48

Fix rebase

9a7f911

jreback requested changes Nov 2, 2019

View reviewed changes

jreback added this to the 1.0 milestone Nov 2, 2019

MarcoGorelli added 2 commits November 2, 2019 16:21

Stylistic changes

395f04b

Finish stylistic changes

44a6373

jreback approved these changes Nov 2, 2019

View reviewed changes

jreback merged commit bf5848f into pandas-dev:master Nov 2, 2019

MarcoGorelli deleted the tz-localize-categories branch November 12, 2019 11:25

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019

BUG: make tz_localize operate on values rather than categories (panda…

b6bb702

…s-dev#28300)

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG: make tz_localize operate on values rather than categories (panda…

e256811

…s-dev#28300)

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG: make tz_localize operate on values rather than categories (panda…

5f653df

…s-dev#28300)

kurchi1205 mentioned this pull request Aug 26, 2021

BUG: dt.tz_convert() on categorical[datetime] resets index #43080

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: make tz_localize operate on values rather than categories #28300

BUG: make tz_localize operate on values rather than categories #28300

MarcoGorelli commented Sep 5, 2019 •

edited

Loading

jbrockmendel Sep 5, 2019

MarcoGorelli Sep 5, 2019 •

edited

Loading

jbrockmendel Sep 5, 2019

MarcoGorelli Sep 7, 2019

jreback Sep 7, 2019

MarcoGorelli Sep 10, 2019

jbrockmendel Sep 11, 2019

jreback Sep 7, 2019

jreback Sep 7, 2019

jreback commented Sep 7, 2019

MarcoGorelli commented Sep 16, 2019

jbrockmendel Sep 16, 2019

jbrockmendel Sep 16, 2019

jbrockmendel Sep 16, 2019

jbrockmendel Sep 16, 2019

MarcoGorelli Sep 17, 2019

MarcoGorelli Sep 20, 2019 •

edited

Loading

jreback Sep 18, 2019

jreback Sep 18, 2019

TomAugspurger Sep 23, 2019

MarcoGorelli Sep 24, 2019

TomAugspurger Sep 24, 2019

MarcoGorelli Sep 24, 2019

MarcoGorelli commented Oct 8, 2019

jreback commented Oct 8, 2019

jbrockmendel commented Nov 2, 2019

MarcoGorelli commented Nov 2, 2019

jreback left a comment

jreback Nov 2, 2019

jreback Nov 2, 2019

MarcoGorelli commented Nov 2, 2019

jreback commented Nov 2, 2019

	- Using date accessors on a categorical dtyped :class:`Series` of datetimes was not returning the same object that would
	- Bug in :class:`Series.str` and :class:`Series.dt` accessor for categorical-dtype returning results with the incorrect length (:issue:`27952`)

BUG: make tz_localize operate on values rather than categories #28300

BUG: make tz_localize operate on values rather than categories #28300

Conversation

MarcoGorelli commented Sep 5, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoGorelli Sep 5, 2019 • 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

jreback commented Sep 7, 2019

MarcoGorelli commented Sep 16, 2019

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

MarcoGorelli Sep 20, 2019 • 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

MarcoGorelli commented Oct 8, 2019

jreback commented Oct 8, 2019

jbrockmendel commented Nov 2, 2019

MarcoGorelli commented Nov 2, 2019

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli commented Nov 2, 2019

jreback commented Nov 2, 2019

MarcoGorelli commented Sep 5, 2019 •

edited

Loading

MarcoGorelli Sep 5, 2019 •

edited

Loading

MarcoGorelli Sep 20, 2019 •

edited

Loading