-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
handle DST appropriately in Timestamp.replace #18618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
cfb760c
4ca60fd
f738989
98a7296
6f36a97
b916b17
b00d763
000f6cd
ec9b38a
f5886e2
4cfd949
a3b14f9
504cf97
4459c9f
1eb5f2f
dced361
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,4 +248,4 @@ Other | |
- Fixed a bug where creating a Series from an array that contains both tz-naive and tz-aware values will result in a Series whose dtype is tz-aware instead of object (:issue:`16406`) | ||
- Fixed construction of a :class:`Series` from a ``dict`` containing ``NaN`` as key (:issue:`18480`) | ||
- Adding a ``Period`` object to a ``datetime`` or ``Timestamp`` object will now correctly raise a ``TypeError`` (:issue:`17983`) | ||
- | ||
- :func:`Timestamp.replace` will now handle Daylight Savings transitions gracefully (:issue:`18319`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about "will no longer crash when handling DST transitions" ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. crash is a not an appropriate term for user notes. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1229,6 +1229,26 @@ def f(): | |
dt = Timestamp('2013-11-03 01:59:59.999999-0400', tz='US/Eastern') | ||
assert dt.tz_localize(None) == dt.replace(tzinfo=None) | ||
|
||
def test_replace_across_dst(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you tests with dateutil as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test is pretty specific to pytz API There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test is specific to the Rather than using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this a suggestion for this test or more generally? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this test at least, since with tests you can always engineer your datetime literals such that you never have to use anything except It's also just a useful piece of information to have, that In this case you know what the correct answer should be, in absolute time, so you can just declare your initial variable as the correct answer in Of course, this method will fail if you try to construct an imaginary time, since there is no mapping between UTC and imaginary times. |
||
# GH#18319 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you provide a 1-liner about what this is testing (I know the title is informative, but a little color) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure |
||
tz = pytz.timezone('US/Eastern') | ||
|
||
ts_naive = Timestamp('2017-12-03 16:03:30') | ||
ts_aware = tz.localize(ts_naive) | ||
|
||
# Preliminary sanity-check | ||
assert ts_aware == ts_aware.tzinfo.normalize(ts_aware) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to parametrize this across dateutil & pytz, so you need to move this to |
||
# Replace across DST boundary | ||
ts2 = ts_aware.replace(month=6) | ||
|
||
# Check that `replace` preserves hour literal | ||
assert (ts2.hour, ts2.minute) == (ts_aware.hour, ts_aware.minute) | ||
|
||
# Check that post-replace object is appropriately normalized | ||
ts2b = ts2.tzinfo.normalize(ts2) | ||
assert ts2 == ts2b | ||
|
||
def test_ambiguous_compat(self): | ||
# validate that pytz and dateutil are compat for dst | ||
# when the transition happens | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have rebase issues here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix. Not a big deal, but for changes limited to whatsnew is skipping the CI a viable option? The CI turnaround time is close to a full day and that file gets touched a lot.