ENH: add fold support to Timestamp constructor #31563

AlexKirko · 2020-02-02T08:10:40Z

PERF Note: Current implementation slows down the Timestamp constructor. I've tested this thoroughly and tracked it down to ba7fcd5 where I changed the function signatures at the very beginning of working on the PR. The performance overhead appeared before any of the logic was implemented.

closes API/ENH: Support/document/test fold argument in Timestamp #25057, closes BUG: Timestamp UTC offset incorrect for dateutil tz in ambiguous DST time #31338
tests added 32 / passed 32
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Reasoning for changes

We currently don't support fold from PEP 495, and this causes us a lot of grief, including, but not limited to, broken Timestamp representations at the edge of DST shift (see #31338). The values can also break.

Support of the fold attribute helps us deal with that. Now, if the wall clock time occurs twice due to a DST shift, we will know exactly at which point in time we are.

This also removes inconsistencies between using pytz and dateutil: now both the values and representations match for both timezone packages near DST summer/winter shifts.

Scope of PR

Implementing fold into Timestamp can easily get out of hand. Parent pydatetime has it easy, as the object doesn't need to sync its underlying epoch time, fold, and the representation, like we do.

This PR is already large, so I propose we limit its scope to minimal consistent fold support:

when fold is explicitly supplied, attempt to shift value across the DST boundary according to the value supplied
infer fold from value during tz-aware Timestamp construction. For example, if Timestamp is built from epoch time, then we can infer fold.
pass the resulting fold to the underlying datetime constructor in create_timestamp_from_ts so that it gets stored in the object (can't assign it directly as it's read-only).
implement local timezone support for fold. Don't have any idea how to do this though.

Things I suggest we leave to discussion and other PRs:

conflicts resolution. A user can supply fold=1 for a datetime that is nowhere near a DST shift. We can raise and Error or a Warning in the future. For now, we check whether fold can be 1, and if not, we leave it as default fold=0.
consider reintroducing ambiguous time errors. Currently, the implementation assumes fold == 0 for ambiguous time like Timestamp("2019-10-27 01:30:00", tz='Europe/London'). The error was dropped to mirror the fold=0 default in pydatetime and to allow the fold attribute to be bint in Cython functions.

Note: pydatetime doesn't infer fold from value and doesn't raise errors when you assign fold=1 nowhere near a fold. Example:

from datetime import datetime
from dateutil.tz import gettz

dt = datetime.datetime(2015, 10, 27, 5, 30, 0, 0, gettz("Europe/London"), fold=1)
dt.fold
OUT:
1

Performance problems

This implementation behaves as I expect it to, but it slows down scalar constructor benchmarks by 30-70%, even for tz-naive benchmarks.

Update: since the benchmark slowdown appeared before any of the functionality, this has nothing to do with the logic introduced and all to do with adding bint fold to function signatures, apparently. I'm afraid I don't know Cython well enough to research further.

Default behaviour near DST boundary is now for fold=0

AlexKirko · 2020-02-23T13:38:03Z

@jreback
Sorry if this PR is waiting for its turn. Pinging you in case it fell through the cracks.

jreback

question and some minor comments. @mroeschke and @pganssle ok here?

jreback · 2020-02-23T14:46:58Z

doc/source/whatsnew/v1.1.0.rst

+                      tz="dateutil/Europe/London", fold=1)
+    ts
+
+For more, see :ref:`Timezone section <timeseries.timezone>` in the user guide on working with timezones.


reference the sub-section here

jreback · 2020-02-23T14:47:24Z

doc/source/user_guide/timeseries.rst

@@ -2220,6 +2220,32 @@ you can use the ``tz_convert`` method.

   rng_pytz.tz_convert('US/Eastern')

+.. note::


let's make this a sub-section (e.g. Fold), add a reference (so you can link to it in whatsnew)

should be the same level as working with time zones

jreback · 2020-02-23T14:51:24Z

pandas/_libs/tslibs/timestamps.pyx

+                    "timezones."
+                )
+
+            if hasattr(ts_input, 'fold'):


is this needed? (e.g. we are passing fold now to all of the construction functions)

It's still needed when we pass a naive datetime, fold, and a timezone.

jreback · 2020-02-23T14:53:38Z

also pls merge master as some cosmetic changes where done in cython. ping on green.

mroeschke · 2020-02-24T06:23:13Z

pandas/_libs/tslibs/timestamps.pyx

+        # Allow fold only for unambiguous input
+        if fold is not None:
+            if fold not in [0, 1]:
+                raise ValueError(


Could you add a test that hits this exception?

mroeschke

Sorry I've been out of the loop on this PR.

Looks pretty good from the surface. Just one comment.

AlexKirko · 2020-02-25T07:13:08Z

Linux 32 bit azure pipeline fails to build for everyone: #32229
Doesn't look like it has anything to do with this PR.

AlexKirko · 2020-02-25T09:32:06Z

UPDATE: outdated, see below
All green except failures that don't have anything to do with this PR.

Linux py36_32bit fails to build and is being handled in issue #32229. macOS py36_macos has failed test_value_counts_unique_nunique, which is being handled in issue #32220 and PR #32201

Docs changed (tested ref locally, and everything seems to work), test added for invalid fold values, master merged. ts_input.replace within hasattr if block is still necessary, because it sets fold for naive-datetime ts_input passed together with fold and a timezone.

Resolve conflict in timestamps.pyx by accepting the master version.

AlexKirko · 2020-02-26T07:03:22Z

@jreback
All green now. Merged in new changes that fix broken CI.

Changes you requested made.

ts_input.replace within hasattr if block is still necessary, because it sets fold for naive-datetime ts_input passed together with fold and a timezone.

jreback · 2020-02-26T12:35:24Z

thanks @AlexKirko very nice! and thanks to @pganssle and @mroeschke for the lively discussion!

AlexKirko · 2020-02-26T16:16:43Z

@jreback @pganssle @mroeschke
Great! Thanks to everyone for all the effort you made to make the changes as robust as possible. We didn't hit exactly 256 comments or 128 commits, but other than that I'm very happy with the result.
A special thank you to @pganssle for the in-depth research and discussion!

pganssle · 2020-02-26T16:24:14Z

Sorry I couldn't give this one last once-over, but thanks so much for putting in the hard work to get this right, @AlexKirko.

And now there's PEP 495 support in pandas, just in time for PEP 615 (discussion, currently underway)!

mroeschke · 2020-02-26T19:16:34Z

Great work @AlexKirko! Really happy to have this in.

AlexKirko added 30 commits February 1, 2020 00:02

TST: add basic test for construction with fold

84bfff2

ENH: add basic fold support

ba7fcd5

ENH: add fold to ts properties

0b6f894

baseline fold in conversion

5c58b3a

add fold placeholder to conversion

546789a

add fold to convert_to_tsobject

fc69bbb

TST: add test to infer fold from value

57d42b3

ENH: infer fold from value for dateutil

f2ad196

ENH: infer fold for pytz (loss in performance)

935a3ec

PERF: remove unnecessary ifs

d35af8f

check that we are not at the left edge of deltas

e6d4aaa

ENH: adjust Timestamp.value for fold (if not from datetime or string)

fd98b27

CLN: remove extra spaces

af86f79

TST: add test for adjusting value for fold

a6965e9

DOC: add to comment for local timezone

4caf9bb

TST: add string case to fold inferring test

6843ed2

ENH: infer fold for timestamp from string

ebbf21f

TST: add fold value adjustment from string

4f43638

ENH: adjust value for fold from string

6238f9b

basic from datetime fold support with bugs

3584791

complete adjust value for fold from datetime

237341b

use input fold as default

12b8b4e

TST: adjust datetime for included fold

92e990e

TST: infer fold from fold included in datetime

c2189d3

FIX: fix bool condition for inferring fold for str

9c9c2dd

remove unnecessary value shift for inferring fold from str

f0bbbcb

remove unnecessary GH24329 fix

ca46078

pass fold through Timstamp.replace

411b036

TST:fix ambiguous compatibility test in scalar test_timezones

464dadf

TST: remove test for ambiguous error near dst boundary

2b9f2f6

Default behaviour near DST boundary is now for fold=0

AlexKirko requested review from jbrockmendel and jreback February 19, 2020 12:14

DOC: give descriptive names to refs

46a279b

jreback added this to the 1.1 milestone Feb 23, 2020

jreback requested changes Feb 23, 2020

View reviewed changes

mroeschke reviewed Feb 24, 2020

View reviewed changes

AlexKirko added 3 commits February 25, 2020 09:32

DOC: make fold a subsection in timeseries.rst

81560bb

TST: add test for invalid fold raise

4256642

Merge branch 'master' into add-fold-to-timestamp

a24594a

DOC: rephrase text in timeseries.rst to improve readability

0168aa6

AlexKirko added 2 commits February 26, 2020 09:15

Merge branch 'master' into add-fold-to-timestamp

3a605c3

Resolve conflict in timestamps.pyx by accepting the master version.

add fold support to merged code

cd02318

AlexKirko requested a review from jreback February 26, 2020 07:03

jreback approved these changes Feb 26, 2020

View reviewed changes

jreback merged commit 6e04264 into pandas-dev:master Feb 26, 2020

roberthdevries pushed a commit to roberthdevries/pandas that referenced this pull request Mar 2, 2020

ENH: add fold support to Timestamp constructor (pandas-dev#31563)

cbc9d41

AlexKirko deleted the add-fold-to-timestamp branch March 6, 2020 08:24

mroeschke mentioned this pull request Mar 7, 2020

date_range returns duplicated dates when used with dateutil over a range that includes the end of daylight saving time #24373

Closed

hasB4K mentioned this pull request May 2, 2020

BUG: hash of Timestamp on fold=1 create a Segfault #33931

Closed

3 tasks

mroeschke mentioned this pull request Mar 23, 2023

BUG: Timestamp fails when fold is passed and building from components as positional args (Pandas 2.0 regression) #52117

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add fold support to Timestamp constructor #31563

ENH: add fold support to Timestamp constructor #31563

AlexKirko commented Feb 2, 2020 •

edited

Loading

AlexKirko commented Feb 23, 2020

jreback left a comment

jreback Feb 23, 2020

AlexKirko Feb 25, 2020

jreback Feb 23, 2020

jreback Feb 23, 2020

AlexKirko Feb 25, 2020

jreback Feb 23, 2020

AlexKirko Feb 25, 2020

jreback commented Feb 23, 2020

mroeschke Feb 24, 2020

AlexKirko Feb 25, 2020

mroeschke left a comment

AlexKirko commented Feb 25, 2020 •

edited

Loading

AlexKirko commented Feb 25, 2020 •

edited

Loading

AlexKirko commented Feb 26, 2020

jreback commented Feb 26, 2020

AlexKirko commented Feb 26, 2020

pganssle commented Feb 26, 2020

mroeschke commented Feb 26, 2020

		@@ -2220,6 +2220,32 @@ you can use the ``tz_convert`` method.

		rng_pytz.tz_convert('US/Eastern')

		.. note::

ENH: add fold support to Timestamp constructor #31563

ENH: add fold support to Timestamp constructor #31563

Conversation

AlexKirko commented Feb 2, 2020 • edited Loading

Reasoning for changes

Scope of PR

Performance problems

AlexKirko commented Feb 23, 2020

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

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 Feb 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

AlexKirko commented Feb 25, 2020 • edited Loading

AlexKirko commented Feb 25, 2020 • edited Loading

AlexKirko commented Feb 26, 2020

jreback commented Feb 26, 2020

AlexKirko commented Feb 26, 2020

pganssle commented Feb 26, 2020

mroeschke commented Feb 26, 2020

AlexKirko commented Feb 2, 2020 •

edited

Loading

AlexKirko commented Feb 25, 2020 •

edited

Loading

AlexKirko commented Feb 25, 2020 •

edited

Loading