BUG: Fix Timestamp constructor changes value on ambiguous DST #30995

AlexKirko · 2020-01-14T07:04:46Z

closes BUG: Timestamp(Timestamp(Ambiguous time)) modifies .value with dateutil tz #24329
1 tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

AlexKirko · 2020-01-14T07:13:07Z

This took some digging, so please bear with me.

Short version
When we make a Timestamp using epoch time that corresponds to the second time an ambigous DST time occurs, the Timestamp gets created and gets assigned a correct Timestamp.value. However, when we call the Timestamp constructor again on this Timestamp, this is what happens:

convert_datetime_to_tsobject in conversion.pyx gets called.
It eventually calls npy_datetimestruct_to_datetime which doesn't care about DST. As it simply adds up date components after converting them to seconds.
Then we attempt to shift the Timestamp.value using get_utcoffset. It sees the state its arguments are in, and, if we use dateutil assumes that we are in DST. pytz assumes we aren't in DST, which means that only dateutil breaks, because we end up shifting Timezone.value by DST timedelta (npy_datetimestruct_to_datetime doesn't shift, and then get_utcoffset shifts).

Notes
The core of the problem is that when we go from summer to winter time in DST timezones, the same time occurs twice as the clock is moved back one hour at a certain time. In Europe/London (which is used in the test), the clocks are moved at 27th of November 2:00 A.M. This means that we get 1:00 A.M. twice: before and after the shift.
dateutil handles the move differently from pytz here. For the ambiguous period it assumes that we are still in DST, and this is why the bug manifested only with dateutil timezones.

PS
I've come upon another bug while working on this, but I think it's out-of-scope for this PR and would like to handle it in a new issue and PR:

IN:
import pandas as pd
t = pd.Timestamp('2013-10-27 01:00:00+0000', tz='dateutil/Europe/London')
t

OUT:
Timestamp('2013-10-27 01:00:00+0100', tz='dateutil/GB-Eire')

AlexKirko · 2020-01-14T07:52:58Z

Wasn't able to reproduce the test fail on Windows. Will need to set up a dev environment in Linux and try to reproduce it. Ideas on why this might be happening would be appreciated.
Update: for some reason a dateutil timezone object belongs to different classes on Windows and Linux/Mac. Both classes are called tzfile which makes aliases in the import section necessary.
Update 2: imported dateutil.tz.tzfile instead, same way it's used in timezones.pyx. Works like a charm.

pandas/_libs/tslibs/conversion.pyx

+        # Two check in if necessary because class
+        # differs on Windows and Linux
+        if (isinstance(ts.tzinfo, du_tzfile1) or
+                isinstance(ts.tzinfo, du_tzfile2)):


AlexKirko · 2020-01-14T14:31:24Z

pandas/_libs/tslibs/conversion.pyx

+                isinstance(ts.tzinfo, du_tzfile2)):
+            if ts.tzinfo.is_ambiguous(ts):
+                dst_offset = ts.tzinfo.dst(ts)
+                obj.value += int(dst_offset.total_seconds() * 1e9)


Because pydatetime_to_dt64 doesn't take DST into account but get_utcoffset does, we need to add the DST timedelta to the result of pydatetime_to_dt64. This is necessary only for dateutil, because for ambiguous dates pytz immediately switches off DST and reduces UTC offset, so no correction is necessary for pytz timezones.

can you add a comment to this effect here

AlexKirko · 2020-01-14T14:32:57Z

pandas/tests/indexes/datetimes/test_timezones.py

-            assert times[-1] == Timestamp("2013-10-27 01:00:00+0100", tz=tz, freq="H")
-        else:
-            assert times[-1] == Timestamp("2013-10-27 01:00:00+0000", tz=tz, freq="H")
+        assert times[-1] == Timestamp("2013-10-27 01:00:00+0000", tz=tz, freq="H")


Now no matter whether we use dateutil or pytz timezones, we get the same date range, so separate testing conditions are no longer necessary.

AlexKirko · 2020-01-14T16:08:36Z

pandas/_libs/tslibs/conversion.pyx

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

+from dateutil.tz import tzfile as _dateutil_tzfile


This is used in timezones.pyx when we need to check for dateutil timezone and make sure it works both on Windows and Linux. Seems to work.

mroeschke · 2020-01-14T16:19:48Z

pandas/_libs/tslibs/conversion.pyx

+        # GH 24329 Take DST offset into account
+        # use dateutil.tz.tzfile to check type
+        if ts.tzinfo is not None:
+            if isinstance(ts.tzinfo, _dateutil_tzfile):


Think you can usetreat_tz_as_dateutil

pandas/pandas/_libs/tslibs/timezones.pyx

Line 42 in d78c061

cdef inline bint treat_tz_as_dateutil(object tz):

And can also remove the if ts.tzinfo is not None check if you use treat_tz_as_dateutil directly.

mroeschke · 2020-01-14T16:21:39Z

Does this also fix the case for nonexistent times near DST?

#24329 (comment)

AlexKirko · 2020-01-14T16:29:23Z

@mroeschke Unfortunately, no. Shifts it back by an hour. I would have to look into why this happens.

AlexKirko · 2020-01-14T17:55:17Z

@mroeschke Okay, looked into it. You'll be happy to know that, according to dateutil, summer time starts 128 nanoseconds before 2 A.M. Looks to be a bug in dateutil that isn't connected to the issue that this PR is trying to handle. The bug manifests starting with 128 seconds before 2 A.M. both on master and my branch.
Details
This is fine:

>>> pd.__version__
'0.26.0.dev0+1773.g664d928fd'
>>> epoch =  1552211999999999871
>>> t = pd.Timestamp(epoch, tz='dateutil/US/Pacific')
>>> t
Timestamp('2019-03-10 01:59:59.999999871-0800', tz='dateutil/US/Pacific')
>>> t.value
1552211999999999871
>>> pd.Timestamp(t)
Timestamp('2019-03-10 01:59:59.999999871-0800', tz='dateutil/US/Pacific')
>>> pd.Timestamp(t).value
1552211999999999871

This is also fine:

>>> epoch =  1552212000000000000
>>> t = pd.Timestamp(epoch, tz='dateutil/US/Pacific')
>>> t
Timestamp('2019-03-10 03:00:00-0700', tz='dateutil/US/Pacific')
>>>
>>> t.value
1552212000000000000
>>> pd.Timestamp(t)
Timestamp('2019-03-10 03:00:00-0700', tz='dateutil/US/Pacific')
>>> pd.Timestamp(t).value
1552212000000000000

Meanwhile, this breaks representation and gets us nonexistent times:

>>> epoch =  1552211999999999872
>>> t = pd.Timestamp(epoch, tz='dateutil/US/Pacific')
>>> t
Timestamp('2019-03-10 01:59:59.999999872-0700', tz='dateutil/US/Pacific')
>>> t.value
1552211999999999872
>>> pd.Timestamp(t)
Timestamp('2019-03-10 01:59:59.999999872-0800', tz='dateutil/US/Pacific')
>>> pd.Timestamp(t).value
1552208399999999872

And right on the cusp, the value breaks too:

>>> epoch =  1552211999999999999
>>> t = pd.Timestamp(epoch, tz='dateutil/US/Pacific')
>>> t
Timestamp('2019-03-10 01:59:59.999999999-0700', tz='dateutil/US/Pacific')
>>> t.value
1552211999999999999
>>> pd.Timestamp(t)
Timestamp('2019-03-10 01:59:59.999999999-0800', tz='dateutil/US/Pacific')
>>> pd.Timestamp(t).value
1552208399999999999

So the bug is there, but it turns out to be unrelated and matters only for 128 nanoseconds before the switch to summer time.
I can make a separate issue, but frankly, I think it should be a dateutil issue. The chances that a user will encounter this are miniscule, so it's not like we have to compensate for dateutil.

AlexKirko · 2020-01-14T18:09:14Z

pandas/_libs/tslibs/conversion.pyx

@@ -382,6 +382,11 @@ cdef _TSObject convert_datetime_to_tsobject(datetime ts, object tz,
            obj.tzinfo = tz
    else:
        obj.value = pydatetime_to_dt64(ts, &obj.dts)
+        # GH 24329 Take DST offset into account
+        if treat_tz_as_dateutil(ts.tzinfo):


@mroeschke Switched to treat_tz_as_dateutil. Much cleaner now, thank you.

mroeschke

Nice investigative work! LGTM.

Yes, please make a separate issue for the nonexistent cases. Your examples are really good.

mroeschke · 2020-01-14T19:36:39Z

doc/source/whatsnew/v1.0.0.rst

@@ -968,6 +968,7 @@ Datetimelike
 - Bug in :func:`date_range` with custom business hours as ``freq`` and given number of ``periods`` (:issue:`30593`)
 - Bug in :class:`PeriodIndex` comparisons with incorrectly casting integers to :class:`Period` objects, inconsistent with the :class:`Period` comparison behavior (:issue:`30722`)
 - Bug in :meth:`DatetimeIndex.insert` raising a ``ValueError`` instead of a ``TypeError`` when trying to insert a timezone-aware :class:`Timestamp` into a timezone-naive :class:`DatetimeIndex`, or vice-versa (:issue:`30806`)
+- Bug in :class:`Timestamp` where constructing :class:`Timestamp` from ambiguous epoch time and calling constructor again changed :meth:`Timestamp.value` property (:issue:`24329`)


Sorry one more thing, can you move to whatsnew version 1.1.0?

jreback

minor comments, otherwise lgtm (ex moving the whatsnew to 1.1). @mroeschke pls merge when satisfied.

jreback · 2020-01-15T03:20:32Z

pandas/_libs/tslibs/conversion.pyx

+                isinstance(ts.tzinfo, du_tzfile2)):
+            if ts.tzinfo.is_ambiguous(ts):
+                dst_offset = ts.tzinfo.dst(ts)
+                obj.value += int(dst_offset.total_seconds() * 1e9)


can you add a comment to this effect here

AlexKirko · 2020-01-15T07:40:35Z

pandas/_libs/tslibs/conversion.pyx

+        # GH 24329 When datetime is ambiguous,
+        # pydatetime_to_dt64 doesn't take DST into account
+        # but with dateutil timezone, get_utcoffset does
+        # so we need to correct for it


Added a comment explaining the reasoning for this solution. Tried to make it as brief as possible.

AlexKirko · 2020-01-15T07:40:58Z

doc/source/whatsnew/v1.1.0.rst

@@ -59,6 +59,7 @@ Categorical

 Datetimelike
 ^^^^^^^^^^^^
+- Bug in :class:`Timestamp` where constructing :class:`Timestamp` from ambiguous epoch time and calling constructor again changed :meth:`Timestamp.value` property (:issue:`24329`)


Moved to whatsnew for v1.1.0

AlexKirko · 2020-01-15T08:15:19Z

@mroeschke Made the required changes. Also performed the final cleanup: rebased to updated master and squashed tiny commits.
Please take a look if it's ready for merging.

mroeschke · 2020-01-15T16:33:28Z

Thanks @AlexKirko!

If you'd like to continue investigating #31043, it would be much appreciated.

test_dti_construction_nonexistent_endpoint had an expected fail because of unresolved 24329. This fix in conjunction with pandas-dev#30995 means it now returns the expected value and does not fail

AlexKirko force-pushed the FIX-timestamp-constructor-ambiguous branch from b4bec5f to 2abc37b Compare January 14, 2020 07:27

AlexKirko mentioned this pull request Jan 14, 2020

PERF: add shortcut to Timestamp constructor #30676

Merged

5 tasks

AlexKirko commented Jan 14, 2020

View reviewed changes

pandas/_libs/tslibs/conversion.pyx Outdated

# Two check in if necessary because class

# differs on Windows and Linux

if (isinstance(ts.tzinfo, du_tzfile1) or

isinstance(ts.tzinfo, du_tzfile2)):

This comment was marked as outdated.

Sign in to view

AlexKirko commented Jan 14, 2020

View reviewed changes

mroeschke reviewed Jan 14, 2020

View reviewed changes

mroeschke added Timezones Timezone data dtype Bug labels Jan 14, 2020

AlexKirko commented Jan 14, 2020

View reviewed changes

AlexKirko requested a review from mroeschke January 14, 2020 18:37

mroeschke approved these changes Jan 14, 2020

View reviewed changes

mroeschke reviewed Jan 14, 2020

View reviewed changes

mroeschke added this to the 1.1 milestone Jan 14, 2020

jreback approved these changes Jan 15, 2020

View reviewed changes

AlexKirko added 4 commits January 15, 2020 10:14

BUG: fix Timestamp constructor value change on DST Timestamp

2aa90a0

BUG: expand if check for Linux

b4fd497

CLN: switch from isinstance to treat_tz_asdateutil

5053889

CLN: add comment to solution and move to v1.1.0

322178c

AlexKirko force-pushed the FIX-timestamp-constructor-ambiguous branch from 2a14b57 to 322178c Compare January 15, 2020 07:25

DOC: reword bugfix comment

85767af

AlexKirko force-pushed the FIX-timestamp-constructor-ambiguous branch from d980836 to 85767af Compare January 15, 2020 07:39

AlexKirko commented Jan 15, 2020

View reviewed changes

AlexKirko requested a review from mroeschke January 15, 2020 10:49

AlexKirko mentioned this pull request Jan 15, 2020

BUG: nonexistent Timestamp pre-summer/winter DST change with dateutil timezone #31043

Closed

mroeschke approved these changes Jan 15, 2020

View reviewed changes

mroeschke merged commit bc9d329 into pandas-dev:master Jan 15, 2020

AlexKirko deleted the FIX-timestamp-constructor-ambiguous branch January 16, 2020 12:40

AlexKirko mentioned this pull request Jan 27, 2020

BUG: Timestamp UTC offset incorrect for dateutil tz in ambiguous DST time #31338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix Timestamp constructor changes value on ambiguous DST #30995

BUG: Fix Timestamp constructor changes value on ambiguous DST #30995

AlexKirko commented Jan 14, 2020

AlexKirko commented Jan 14, 2020 •

edited

Loading

AlexKirko commented Jan 14, 2020 •

edited

Loading

This comment was marked as outdated.

AlexKirko Jan 14, 2020

jreback Jan 15, 2020

AlexKirko Jan 14, 2020

AlexKirko Jan 14, 2020

mroeschke Jan 14, 2020

mroeschke Jan 14, 2020

mroeschke commented Jan 14, 2020

AlexKirko commented Jan 14, 2020

AlexKirko commented Jan 14, 2020 •

edited

Loading

AlexKirko Jan 14, 2020

mroeschke left a comment •

edited

Loading

mroeschke Jan 14, 2020

jreback left a comment

jreback Jan 15, 2020

AlexKirko Jan 15, 2020

AlexKirko Jan 15, 2020

AlexKirko commented Jan 15, 2020

mroeschke commented Jan 15, 2020

BUG: Fix Timestamp constructor changes value on ambiguous DST #30995

BUG: Fix Timestamp constructor changes value on ambiguous DST #30995

Conversation

AlexKirko commented Jan 14, 2020

AlexKirko commented Jan 14, 2020 • edited Loading

AlexKirko commented Jan 14, 2020 • edited Loading

This comment was marked as outdated.

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

mroeschke commented Jan 14, 2020

AlexKirko commented Jan 14, 2020

AlexKirko commented Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

mroeschke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

AlexKirko commented Jan 15, 2020

mroeschke commented Jan 15, 2020

AlexKirko commented Jan 14, 2020 •

edited

Loading

AlexKirko commented Jan 14, 2020 •

edited

Loading

AlexKirko commented Jan 14, 2020 •

edited

Loading

mroeschke left a comment •

edited

Loading