Skip to content

BUG: Fixed inconsistent offset behavior for series #43784 #48129

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

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/source/whatsnew/v1.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -992,8 +992,9 @@ Datetimelike
- Bug in :meth:`DatetimeIndex.resolution` incorrectly returning "day" instead of "nanosecond" for nanosecond-resolution indexes (:issue:`46903`)
- Bug in :class:`Timestamp` with an integer or float value and ``unit="Y"`` or ``unit="M"`` giving slightly-wrong results (:issue:`47266`)
- Bug in :class:`.DatetimeArray` construction when passed another :class:`.DatetimeArray` and ``freq=None`` incorrectly inferring the freq from the given array (:issue:`47296`)
- Bug in :func:`to_datetime` where ``OutOfBoundsDatetime`` would be thrown even if ``errors=coerce`` if there were more than 50 rows (:issue:`45319`)
- Bug when adding a :class:`DateOffset` to a :class:`Series` would not add the ``nanoseconds`` field (:issue:`47856`)
- Bug where adding a :class:`DateOffset` to a :class:`DatetimeIndex` or :class:`Series` over a Daylight Savings Time boundary would produce an incorrect result (:issue:`43784`)
- Bug in :func:`to_datetime` where ``OutOfBoundsDatetime`` would be thrown even if ``errors=coerce`` if there were more than 50 rows (:issue:`45319`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you undo this reordering?

-

Timedelta
Expand Down
11 changes: 9 additions & 2 deletions pandas/core/arrays/datetimes.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
tz_convert_from_utc,
tzconversion,
)
from pandas._libs.tslibs.offsets import DateOffset
from pandas._typing import npt
from pandas.errors import (
OutOfBoundsDatetime,
Expand Down Expand Up @@ -692,7 +693,10 @@ def _add_offset(self, offset) -> DatetimeArray:
assert not isinstance(offset, Tick)

if self.tz is not None:
values = self.tz_localize(None)
if not offset._use_relativedelta and type(offset) == DateOffset:
values = self.tz_convert("utc").tz_localize(None)
else:
values = self.tz_localize(None)
else:
values = self

Expand All @@ -714,7 +718,10 @@ def _add_offset(self, offset) -> DatetimeArray:
result = DatetimeArray._simple_new(result, dtype=result.dtype)
if self.tz is not None:
# FIXME: tz_localize with non-nano
result = result.tz_localize(self.tz)
if not offset._use_relativedelta and type(offset) == DateOffset:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this only is needed for DateOffset exactly and not its subclasses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Almost all of the subclasses of DateOffset deal with units larger than an hour, so they would not be affected. The only ones that could be realistically affected by this would be BusinessHour and CustomBusinessHour. However, due to the variable nature of the time differences in those classes (eg. adding 1 hour to the very end of a Friday actually ends up adding over 2 whole days to the start of Monday) I think they should not be affected by this change.

Copy link
Member

@mroeschke mroeschke Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will be robust to users subclassing and defining their own DateOffset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't thinking about user-created subclasses in my original response, rather I was thinking of the predefined subclasses like YearEnd. I have it specifically for DateOffset and not its subclasses because this change relies on _use_relativedelta, which signifies offsets in units smaller than a day in DateOffset. DateOffset's subclasses have it always set to a default False, which I absolutely do not want this change to act on. Do you think there is a better way to do this?

result = result.tz_localize("utc").tz_convert(self.tz)
else:
result = result.tz_localize(self.tz)

return result

Expand Down
26 changes: 26 additions & 0 deletions pandas/tests/tseries/offsets/test_dst.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
YearEnd,
)

import pandas as pd
import pandas._testing as tm
from pandas.tests.tseries.offsets.test_offsets import get_utc_offset_hours
from pandas.util.version import Version

Expand Down Expand Up @@ -228,3 +230,27 @@ def test_nontick_offset_with_ambiguous_time_error(original_dt, target_dt, offset
msg = f"Cannot infer dst time from {target_dt}, try using the 'ambiguous' argument"
with pytest.raises(pytz.AmbiguousTimeError, match=msg):
localized_dt + offset


def test_series_dst_addition():
# GH#43784
startdates = pd.Series(
[
Timestamp("2020-10-25", tz="Europe/Berlin"),
Timestamp("2017-03-12", tz="US/Pacific"),
]
)
offset1 = DateOffset(hours=3)
offset2 = DateOffset(days=1)

expected1 = pd.Series(
[Timestamp("2020-10-25 02:00:00+01:00"), Timestamp("2017-03-12 04:00:00-07:00")]
)

expected2 = pd.Series(
[Timestamp("2020-10-26 00:00:00+01:00"), Timestamp("2017-03-13 00:00:00-07:00")]
)

tm.assert_series_equal((startdates + offset1), expected1)

tm.assert_series_equal((startdates + offset2), expected2)