Fix incorrect DTI/TDI indexing; warn before dropping tzinfo #22549

jbrockmendel · 2018-08-31T03:12:23Z

Slight hodge-podge.

#21333 is about to_period method on DatetimeIndex silently dropping timezone info. This PR makes DatetimeIndex.to_period and Timestamp.to_period issue a warning when losing timezone info.

#20464 is entirely unrelated, notes that pd.date_range('1970-01-01', period=2).get_loc(pd.Timedelta(0)) returns 0 instead of raising; ditto for indexing a TDI with a Timestamp.

closes to_period is dropping timezone information #21333
closes BUG: DatetimeIndex allows indexing with Timedelta and likewise for TimedeltaIndex/Timestamp #20464
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2018-08-31T04:27:20Z

Codecov Report

Merging #22549 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22549      +/-   ##
==========================================
+ Coverage   92.05%   92.05%   +<.01%     
==========================================
  Files         169      169              
  Lines       50783    50788       +5     
==========================================
+ Hits        46749    46754       +5     
  Misses       4034     4034

Flag	Coverage Δ
#multiple	`90.46% <100%> (ø)`	⬆️
#single	`42.3% <37.5%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.68% <100%> (+0.02%)`	⬆️
pandas/core/indexes/timedeltas.py	`91.19% <100%> (+0.02%)`	⬆️

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 70c9003...acf7b6e. Read the comment docs.

…_period

…to to_period

gfyoung · 2018-09-01T22:01:27Z

pandas/_libs/tslibs/timestamps.pyx

+        if self.tz is not None:
+            # GH#21333
+            warnings.warn("Converting to Period representation will "
+                          "drop timezone information.")


Warning type and stack level?

This questions applies to everywhere where you placed warnings.

No idea what warning type to use. Suggestions?

As to stack level, I tried a bunch to get that to work with tm.assert_produces_warning and eventually threw in the towel.

As you have it, UserWarning makes sense, but I think being explicit about it is good.

I know what you mean regarding stacklevel. We generally try to get one that makes sense, and if the tests don't cooperate, we can just use check_stacklevel=False.

Since the warnings are raised in direct user-api methods, normally putting a stacklevel=2 should do the correct thing.
What did you not get working in the tests? (which kind of code sample)

What did you not get working in the tests?

IIRC the problem is that the affected code paths issues both the new FutureWarning and in some cases also a PerformanceWarning. tm.assert_produces_warning doesn't support multiple expected warnings, and my attempt to modify it led to tests failing the stacklevel checks.

jreback · 2018-09-04T11:37:42Z

pandas/core/indexes/datetimes.py

@@ -1201,6 +1205,12 @@ def get_loc(self, key, method=None, tolerance=None):
            key = Timestamp(key, tz=self.tz)
            return Index.get_loc(self, key, method, tolerance)

+        if isinstance(key, timedelta):


if/elif here?

jreback · 2018-09-04T11:39:16Z

pandas/core/indexes/timedeltas.py

@@ -487,7 +488,8 @@ def get_loc(self, key, method=None, tolerance=None):
        -------
        loc : int
        """
-        if is_list_like(key):
+        if is_list_like(key) or (isinstance(key, datetime) and key is not NaT):


not sure if we use isna else for NaT checking?

I think in this context isna is less clear. Since there is a specific na-like object we are catching here, we should be explicit about it.

ok this comment looks good

jreback · 2018-09-04T11:39:33Z

pandas/core/indexes/timedeltas.py

@@ -487,7 +488,8 @@ def get_loc(self, key, method=None, tolerance=None):
        -------
        loc : int
        """
-        if is_list_like(key):
+        if is_list_like(key) or (isinstance(key, datetime) and key is not NaT):
+            # GH#20464 for datetime case


make this more explicit, meaning datetime dtype for all-NaT

jreback · 2018-09-04T11:40:12Z

pandas/tests/indexes/datetimes/test_astype.py

+        with tm.assert_produces_warning(UserWarning):
+            # warning that timezone info will be lost
+            result = ts.to_period()[0]
+            expected = ts[0].to_period()



in future PR these should be parameterized

jreback · 2018-09-04T11:40:52Z

pandas/tests/indexes/timedeltas/test_indexing.py

@@ -41,6 +41,14 @@ def test_getitem(self):
            tm.assert_index_equal(result, expected)
            assert result.freq == expected.freq

+    def test_timestamp_invalid_key(self):


can you parameterize over a datetime as well

jreback · 2018-09-04T11:41:20Z

pandas/tests/indexes/timedeltas/test_indexing.py

@@ -41,6 +41,14 @@ def test_getitem(self):
            tm.assert_index_equal(result, expected)
            assert result.freq == expected.freq

+    def test_timestamp_invalid_key(self):
+        # GH#20464
+        tdi = pd.timedelta_range(0, periods=10)


do we have a test that indexes with NaT? (both Timedelta and Datetime dtype)

We have one for timedelta, just added one for datetime

…_period

pep8speaks · 2018-09-05T15:37:08Z

Hello @jbrockmendel! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/indexes/datetimes.py !
There are no PEP8 issues in the file pandas/core/indexes/timedeltas.py !
There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_astype.py !
There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_indexing.py !
There are no PEP8 issues in the file pandas/tests/indexes/timedeltas/test_indexing.py !
There are no PEP8 issues in the file pandas/tests/scalar/timestamp/test_timestamp.py !

jbrockmendel · 2018-09-05T20:51:38Z

Looks like circleCI is a hypothesis timeout

…_period

jreback · 2018-09-07T12:13:47Z

lgtm. rebase after @mroeschke CalendarDay change. ping on green.

…_period

jbrockmendel · 2018-09-07T18:24:29Z

Ping

jreback · 2018-09-08T02:44:23Z

thanks!

…ev#22549)

jbrockmendel added 2 commits August 30, 2018 20:08

Warn before dropping tzinfo, fix incorrect TDI/DTI indexing

dd5cf53

Flake8 fixup

4d34e14

jbrockmendel added 2 commits August 31, 2018 07:52

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

d9cb515

…_period

Merge branch 'to_period' of https://github.com/jbrockmendel/pandas in…

5d2782a

…to to_period

gfyoung added Datetime Datetime data dtype Period Period data type Bug Indexing Related to indexing on series/frames, not to indexes themselves labels Sep 1, 2018

gfyoung reviewed Sep 1, 2018

View reviewed changes

jreback requested changes Sep 4, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Sep 4, 2018

jbrockmendel added 3 commits September 4, 2018 08:37

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

a52af41

…_period

explicit warnings, parametrize tests

0c5e652

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

e9c3dd4

…_period

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

77abd10

…_period

jreback approved these changes Sep 7, 2018

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

acf7b6e

…_period

jreback merged commit 935c5d1 into pandas-dev:master Sep 8, 2018

jbrockmendel deleted the to_period branch September 8, 2018 03:11

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018

Fix incorrect DTI/TDI indexing; warn before dropping tzinfo (pandas-d…

24501d9

…ev#22549)

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

Fix incorrect DTI/TDI indexing; warn before dropping tzinfo (pandas-d…

a87e834

…ev#22549)

h-vetinari mentioned this pull request Oct 2, 2018

TST: the pandas test suite fails on 32bit systems #22813

Closed

mroeschke mentioned this pull request Aug 20, 2019

Bug: Resample removes timezone localization from Period.start_time #28039

Open

joooeey mentioned this pull request May 12, 2022

BUG: PeriodIndex looses tz information without emitting a warning that the timezone is dropped. #47005

Open

3 tasks

joooeey mentioned this pull request Aug 6, 2022

Warn when creating Period with a string that includes timezone information #47990

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect DTI/TDI indexing; warn before dropping tzinfo #22549

Fix incorrect DTI/TDI indexing; warn before dropping tzinfo #22549

jbrockmendel commented Aug 31, 2018

codecov bot commented Aug 31, 2018 •

edited

Loading

gfyoung Sep 1, 2018

gfyoung Sep 1, 2018

jbrockmendel Sep 1, 2018

gfyoung Sep 1, 2018

jorisvandenbossche Sep 5, 2018

jbrockmendel Sep 5, 2018

jreback Sep 4, 2018

jreback Sep 4, 2018

jbrockmendel Sep 4, 2018

jreback Sep 7, 2018

jreback Sep 4, 2018

jreback Sep 4, 2018

jreback Sep 4, 2018

jreback Sep 4, 2018

jbrockmendel Sep 4, 2018

pep8speaks commented Sep 5, 2018

jbrockmendel commented Sep 5, 2018

jreback commented Sep 7, 2018

jbrockmendel commented Sep 7, 2018

jreback commented Sep 8, 2018

Fix incorrect DTI/TDI indexing; warn before dropping tzinfo #22549

Fix incorrect DTI/TDI indexing; warn before dropping tzinfo #22549

Conversation

jbrockmendel commented Aug 31, 2018

codecov bot commented Aug 31, 2018 • edited Loading

Codecov Report

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

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

pep8speaks commented Sep 5, 2018

jbrockmendel commented Sep 5, 2018

jreback commented Sep 7, 2018

jbrockmendel commented Sep 7, 2018

jreback commented Sep 8, 2018

codecov bot commented Aug 31, 2018 •

edited

Loading