-
-
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 1 commit
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 |
---|---|---|
|
@@ -32,7 +32,7 @@ from np_datetime cimport (reverse_ops, cmp_scalar, check_dts_bounds, | |
is_leapyear) | ||
from timedeltas import Timedelta | ||
from timedeltas cimport delta_to_nanoseconds | ||
from timezones cimport get_timezone, is_utc, maybe_get_tz | ||
from timezones cimport get_timezone, is_utc, maybe_get_tz, treat_tz_as_pytz | ||
|
||
# ---------------------------------------------------------------------- | ||
# Constants | ||
|
@@ -927,8 +927,17 @@ class Timestamp(_Timestamp): | |
_tzinfo = tzinfo | ||
|
||
# reconstruct & check bounds | ||
ts_input = datetime(dts.year, dts.month, dts.day, dts.hour, dts.min, | ||
dts.sec, dts.us, tzinfo=_tzinfo) | ||
if _tzinfo is not None and treat_tz_as_pytz(_tzinfo): | ||
# be careful about DST transition, #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. make an informative comments. why woudn't you always localize? 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.
|
||
ts_input = _tzinfo.localize(datetime(dts.year, dts.month, dts.day, | ||
dts.hour, dts.min, dts.sec, | ||
dts.us)) | ||
_tzinfo = ts_input.tzinfo | ||
else: | ||
ts_input = datetime(dts.year, dts.month, dts.day, | ||
dts.hour, dts.min, dts.sec, dts.us, | ||
tzinfo=_tzinfo) | ||
|
||
ts = convert_datetime_to_tsobject(ts_input, _tzinfo) | ||
value = ts.value + (dts.ps // 1000) | ||
if value != NPY_NAT: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1136,6 +1136,26 @@ def test_timestamp(self): | |
dt = ts.to_pydatetime() | ||
assert dt.timestamp() == ts.timestamp() | ||
|
||
def test_replace(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. this can't be the right place, we have lots of replace tests 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. not in test_timestamp... I'll take another look. 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. Looks like its in test_timezones. |
||
# GH#18319 | ||
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) | ||
|
||
# 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 | ||
|
||
|
||
class TestTimestampNsOperations(object): | ||
|
||
|
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.