BUG/TST: Add more timezone fixtures and use is_utc more consistently #23807

mroeschke · 2018-11-20T06:09:59Z

tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Was discussed a while back with @jreback to add more timezones to the tz_aware_fixture, added some less tested ones like:

dateutil.tz.tzlocal
pytz.FixedOffset

This uncovered some bugs with tzlocal especially near DST transitions.

Additionally, using is_utc more consistently for UTC checking across the codebase and no longer passing the string 'UTC' around. Instead the UTC timezone object (i.e. pytz.UTC or dateutil.tz.tzutc()) is passed along. However, for single value conversion to/from UTC, pytz.UTC is used by default.

pep8speaks · 2018-11-20T06:10:07Z

Hello @mroeschke! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/conftest.py !
There are no PEP8 issues in the file pandas/core/arrays/datetimes.py !
There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_construction.py !
There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_timezones.py !
There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_tools.py !
There are no PEP8 issues in the file pandas/tests/scalar/timestamp/test_timestamp.py !
There are no PEP8 issues in the file pandas/tests/scalar/timestamp/test_timezones.py !
There are no PEP8 issues in the file pandas/tseries/frequencies.py !

jbrockmendel · 2018-11-20T14:51:33Z

doc/source/whatsnew/v0.24.0.rst

@@ -1248,6 +1248,8 @@ Timezones
 - Bug when indexing a :class:`Series` with a DST transition (:issue:`21846`)
 - Bug in :meth:`DataFrame.resample` and :meth:`Series.resample` where an ``AmbiguousTimeError`` or ``NonExistentTimeError`` would raise if a timezone aware timeseries ended on a DST transition (:issue:`19375`, :issue:`10117`)
 - Bug in :meth:`DataFrame.drop` and :meth:`Series.drop` when specifying a tz-aware Timestamp key to drop from a :class:`DatetimeIndex` with a DST transition (:issue:`21761`)
+- Bug in :class:`DatetimeIndex` constructor where :class:`NaT` and ``dateutil.tz.tzlocal`` would raise an ``OutOfBoundsDatetime`` error (:issue:`23807`)


nitpick: I'm guessing this happens for users for whom tzlocal is UTC-N but not those for whom tzlocal is UTC+N (@jorisvandenbossche can you check this when convenient?)

This was actually because NaT wasn't handled in tz_localize_to_utc so NaT was converted to its i8 value which was less than Timestamp.min

https://github.com/pandas-dev/pandas/pull/23807/files#diff-66d289312fd0b02e7721bf45fb281797R911

jbrockmendel · 2018-11-20T14:53:53Z

pandas/_libs/tslibs/conversion.pyx

@@ -664,7 +670,8 @@ cdef inline int64_t[:] _tz_convert_dst(int64_t[:] values, tzinfo tz,


 cdef inline int64_t _tz_convert_tzlocal_utc(int64_t val, tzinfo tz,
-                                            bint to_utc=True):
+                                            bint to_utc=True,
+                                            bint from_utc=False):


Isn't from_utc just equal to not to_utc?

Hmm it appears that way in general. I can try to remove the new keyword arg for not to_utc

jbrockmendel · 2018-11-20T15:02:57Z

pandas/conftest.py

@@ -354,7 +354,8 @@ def unique_nulls_fixture(request):


 TIMEZONES = [None, 'UTC', 'US/Eastern', 'Asia/Tokyo', 'dateutil/US/Pacific',
-             'dateutil/Asia/Singapore']
+             'dateutil/Asia/Singapore', tzutc(), tzlocal(), FixedOffset(300),
+             FixedOffset(0), FixedOffset(-300)]


Brainstorm:

non-round numbers for FixedOffset

we have a small number of tests with tzinfos from psycopg2.tz and matplotlib (not sure where in the matplotlib namespace)

random subset of pytz.all_timezones (592 of them)

equivalent from dateutil.tz (not sure off the top of my head how to access that, but if we figure it out I'll make a PR there)

how much additional time do the tests take with these added fixtures? how many more tests

Sure we can add one non-round FixedOffset

matplotlib UTC is just datetime.timezones.utc: https://github.com/matplotlib/matplotlib/blob/9e8be5b0dc0a03f569d7a248f67ef136520adbbb/lib/matplotlib/dates.py#L172

IIRC we just convert psycopg2.tz timezones (from sql functions) directly to UTC, and we don't directly advertise accepting them in other functions. But I guess we do have code that supposedly handles this:

pandas/pandas/_libs/tslibs/timezones.pyx

Line 218 in 3702de2

# psycopg2.tz.FixedOffsetTimezone

As for more pytz and dateutil timezones, I'd be most interested in testing non-conventional timezones, e.g. odd time shifting, but I feel its not necessary to test all of them (baring testing all of them doesn't take that much additional time)

We can add these in a follow up and further discuss.

jreback · 2018-11-20T15:45:11Z

pandas/_libs/tslibs/conversion.pyx

@@ -8,6 +8,7 @@ from numpy cimport int64_t, int32_t, ndarray
 cnp.import_array()

 import pytz
+from dateutil.tz import tzutc


why are you switching this?

No switching, this is just used here for timezone conversion between tzlocal and tzutc (making sure they are both dateutil timezones so they play nice) https://github.com/pandas-dev/pandas/pull/23807/files/de3c83bb946a7791d29a18db5f004228b24c9b02#diff-66d289312fd0b02e7721bf45fb281797R705

jreback · 2018-11-20T15:46:12Z

pandas/conftest.py

@@ -354,7 +354,8 @@ def unique_nulls_fixture(request):


 TIMEZONES = [None, 'UTC', 'US/Eastern', 'Asia/Tokyo', 'dateutil/US/Pacific',
-             'dateutil/Asia/Singapore']
+             'dateutil/Asia/Singapore', tzutc(), tzlocal(), FixedOffset(300),
+             FixedOffset(0), FixedOffset(-300)]


how much additional time do the tests take with these added fixtures? how many more tests

jreback · 2018-11-20T15:46:45Z

pandas/tests/indexes/datetimes/test_tools.py

@@ -409,8 +409,8 @@ def test_to_datetime_tz_pytz(self, cache):
                                                     hour=3, minute=0))],
                       dtype=object)
        result = pd.to_datetime(arr, utc=True, cache=cache)
-        expected = DatetimeIndex(['2000-01-01 08:00:00+00:00',


I find this surprising that you change this

Thanks, yeah this was the wrong fix. Addressed

codecov · 2018-11-22T03:27:16Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #23807      +/-   ##
==========================================
+ Coverage   92.28%   92.28%   +<.01%     
==========================================
  Files         161      161              
  Lines       51500    51501       +1     
==========================================
+ Hits        47528    47529       +1     
  Misses       3972     3972

Flag	Coverage Δ
#multiple	`90.68% <100%> (ø)`	⬆️
#single	`42.31% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`96.98% <ø> (ø)`	⬆️
pandas/core/dtypes/dtypes.py	`95.61% <100%> (ø)`	⬆️
pandas/tseries/frequencies.py	`97.08% <100%> (+0.01%)`	⬆️
pandas/core/arrays/datetimes.py	`98.51% <100%> (ø)`	⬆️
pandas/io/pytables.py	`92.3% <0%> (-0.05%)`	⬇️
pandas/io/formats/printing.py	`93.61% <0%> (+0.53%)`	⬆️

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 e405a10...15ba5fb. Read the comment docs.

jreback · 2018-11-23T03:14:14Z

looks good. does this add a lot more testing? e.g. now that we are having a bigger tz matrix (and use this fixture a fair amount)

mroeschke · 2018-11-23T18:57:14Z

Looking at two, 3.6 travis build, we will go from 36028 tests to 37606 tests (783.49 seconds to 931.50 seconds).

jreback · 2018-11-23T20:45:23Z

thanks! of course if this does close any existing issues, pls update them.

…andas-dev#23807)

Matt Roeschke added 13 commits November 18, 2018 18:24

Add more timezone fixtures

5cf8073

convert 'UTC' to UTC const

7ba3649

Merge remote-tracking branch 'upstream/master' into add_more_timezones

b8bb053

Fix more occurances of 'UTC'

5662f3a

Add notes

2803041

fix tzlocal with iteration

c5e11a6

move codeblock

fbc50da

Merge remote-tracking branch 'upstream/master' into add_more_timezones

cff6e70

Merge remote-tracking branch 'upstream/master' into add_more_timezones

84d8a12

fix tz_localize(None) tzlocal() for Timestamp

f351cd2

Fix tz_localize(None) with tzlocal for DTI

5b97f0d

Add test for tzlocal and NaT

8164688

Add whatsnew

069e6e1

Add issue number and one more fixedoffset

de3c83b

mroeschke changed the title ~~BUG/TST: Add more timezone fixtures and use is_utc more consistently~~ [WIP] BUG/TST: Add more timezone fixtures and use is_utc more consistently Nov 20, 2018

jbrockmendel reviewed Nov 20, 2018

View reviewed changes

jreback requested changes Nov 20, 2018

View reviewed changes

Matt Roeschke added 8 commits November 20, 2018 19:55

Merge remote-tracking branch 'upstream/master' into add_more_timezones

ead3f92

fix dateutil UTC converted to pytz UTC

fb6e96c

Fix some tests

555afaa

don't assume pytz.UTC should localize a datetime

9857897

Merge remote-tracking branch 'upstream/master' into add_more_timezones

00e31d4

isort

c647ba0

extract actual timezone object for DatetimeTZDtype

c219ea3

from_utc -> not to_utc

44a33ab

jreback added the Timezones Timezone data dtype label Nov 21, 2018

mroeschke changed the title ~~[WIP] BUG/TST: Add more timezone fixtures and use is_utc more consistently~~ BUG/TST: Add more timezone fixtures and use is_utc more consistently Nov 21, 2018

Matt Roeschke added 3 commits November 21, 2018 11:48

Merge remote-tracking branch 'upstream/master' into add_more_timezones

a95af1a

Assert already checks timezone

9828aa5

isort

15ba5fb

jreback added this to the 0.24.0 milestone Nov 23, 2018

jreback approved these changes Nov 23, 2018

View reviewed changes

jreback merged commit 94ce05d into pandas-dev:master Nov 23, 2018

mroeschke deleted the add_more_timezones branch November 23, 2018 20:48

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG/TST: Add more timezone fixtures and use is_utc more consistently (p…

03db912

…andas-dev#23807)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG/TST: Add more timezone fixtures and use is_utc more consistently (p…

57f7fa8

…andas-dev#23807)

mroeschke mentioned this pull request Mar 11, 2020

Allow choosing the utc timezone class in pd.to_datetime #32619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG/TST: Add more timezone fixtures and use is_utc more consistently #23807

BUG/TST: Add more timezone fixtures and use is_utc more consistently #23807

mroeschke commented Nov 20, 2018 •

edited

Loading

pep8speaks commented Nov 20, 2018

jbrockmendel Nov 20, 2018

mroeschke Nov 21, 2018

jbrockmendel Nov 20, 2018

mroeschke Nov 21, 2018

jbrockmendel Nov 20, 2018

jreback Nov 20, 2018

mroeschke Nov 21, 2018

mroeschke Nov 21, 2018

jreback Nov 20, 2018

mroeschke Nov 21, 2018

jreback Nov 23, 2018

jreback Nov 20, 2018

jreback Nov 20, 2018

mroeschke Nov 21, 2018

codecov bot commented Nov 22, 2018 •

edited

Loading

jreback commented Nov 23, 2018

mroeschke commented Nov 23, 2018

jreback commented Nov 23, 2018

BUG/TST: Add more timezone fixtures and use is_utc more consistently #23807

BUG/TST: Add more timezone fixtures and use is_utc more consistently #23807

Conversation

mroeschke commented Nov 20, 2018 • edited Loading

pep8speaks commented Nov 20, 2018

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

codecov bot commented Nov 22, 2018 • edited Loading

Codecov Report

jreback commented Nov 23, 2018

mroeschke commented Nov 23, 2018

jreback commented Nov 23, 2018

mroeschke commented Nov 20, 2018 •

edited

Loading

codecov bot commented Nov 22, 2018 •

edited

Loading