BUG: Fixed inconsistent offset behavior for series #43784 #48129
Conversation
Co-authored-by: Matthew Roeschke <[email protected]>
|
I'm not exactly sure how these extra commits got into this new PR, but I fixed the file conflicts so that those extra changes are gone. |
|
I realize |
mroeschke
added
Datetime
|
There seems to be a test failing called |
pandas/core/arrays/datetimes.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
So this only is needed for DateOffset exactly and not its subclasses?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't think this will be robust to users subclassing and defining their own DateOffset
There was a problem hiding this comment.
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?
doc/source/whatsnew/v1.5.0.rst
Outdated
| - Bug when adding a :class:`DateOffset` to a :class:`Series` would not add the ``nanoseconds`` field (:issue:`47856`) | ||
| - Bug in :func:`to_datetime` where ``OutOfBoundsDatetime`` would be thrown even if ``errors=coerce`` if there were more than 50 rows (:issue:`45319`) |
There was a problem hiding this comment.
Could you undo this reordering?
|
@mroeschke I've changed the check to take into account user-created subclasses by removing the type check in the if statement. I've also changed the |
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.