Fix FY5253 onOffset/apply bug, simplify (pandas-dev#18877)

Author: jbrockmendel

doc/source/whatsnew/v0.23.0.txt

@@ -290,6 +290,7 @@ Conversion
 - Bug in :class:`Timestamp` where comparison with an array of ``Timestamp`` objects would result in a ``RecursionError`` (:issue:`15183`)
 - Bug in :class:`WeekOfMonth` and class:`Week` where addition and subtraction did not roll correctly (:issue:`18510`,:issue:`18672`,:issue:`18864`)
 - Bug in :meth:`DatetimeIndex.astype` when converting between timezone aware dtypes, and converting from timezone aware to naive (:issue:`18951`)
+- Bug in :class:`FY5253` where ``datetime`` addition and subtraction incremented incorrectly for dates on the year-end but not normalized to midnight (:issue:`18854`)
 
 
 Indexing

pandas/tests/tseries/offsets/test_fiscal.py

@@ -158,17 +158,6 @@ def test_apply(self):
 
 class TestFY5253NearestEndMonth(Base):
 
-    def test_get_target_month_end(self):
-        assert (makeFY5253NearestEndMonth(
-            startingMonth=8, weekday=WeekDay.SAT).get_target_month_end(
-            datetime(2013, 1, 1)) == datetime(2013, 8, 31))
-        assert (makeFY5253NearestEndMonth(
-            startingMonth=12, weekday=WeekDay.SAT).get_target_month_end(
-            datetime(2013, 1, 1)) == datetime(2013, 12, 31))
-        assert (makeFY5253NearestEndMonth(
-            startingMonth=2, weekday=WeekDay.SAT).get_target_month_end(
-            datetime(2013, 1, 1)) == datetime(2013, 2, 28))
-
     def test_get_year_end(self):
         assert (makeFY5253NearestEndMonth(
             startingMonth=8, weekday=WeekDay.SAT).get_year_end(
@@ -625,3 +614,22 @@ def test_bunched_yearends():
     assert fy.rollback(dt) == Timestamp('2002-12-28')
     assert (-fy).apply(dt) == Timestamp('2002-12-28')
     assert dt - fy == Timestamp('2002-12-28')
+
+
+def test_fy5253_last_onoffset():
+    # GH#18877 dates on the year-end but not normalized to midnight
+    offset = FY5253(n=-5, startingMonth=5, variation="last", weekday=0)
+    ts = Timestamp('1984-05-28 06:29:43.955911354+0200',
+                   tz='Europe/San_Marino')
+    fast = offset.onOffset(ts)
+    slow = (ts + offset) - offset == ts
+    assert fast == slow
+
+
+def test_fy5253_nearest_onoffset():
+    # GH#18877 dates on the year-end but not normalized to midnight
+    offset = FY5253(n=3, startingMonth=7, variation="nearest", weekday=2)
+    ts = Timestamp('2032-07-28 00:12:59.035729419+0000', tz='Africa/Dakar')
+    fast = offset.onOffset(ts)
+    slow = (ts + offset) - offset == ts
+    assert fast == slow

pandas/tseries/offsets.py

@@ -1814,13 +1814,6 @@ def __init__(self, n=1, normalize=False, weekday=0, startingMonth=1,
             raise ValueError('{variation} is not a valid variation'
                              .format(variation=self.variation))
 
-    @cache_readonly
-    def _offset_lwom(self):
-        if self.variation == "nearest":
-            return None
-        else:
-            return LastWeekOfMonth(n=1, weekday=self.weekday)
-
     def isAnchored(self):
         return (self.n == 1 and
                 self.startingMonth is not None and
@@ -1841,6 +1834,8 @@ def onOffset(self, dt):
 
     @apply_wraps
     def apply(self, other):
+        norm = Timestamp(other).normalize()
+
         n = self.n
         prev_year = self.get_year_end(
             datetime(other.year - 1, self.startingMonth, 1))
@@ -1853,32 +1848,26 @@ def apply(self, other):
         cur_year = tslib._localize_pydatetime(cur_year, other.tzinfo)
         next_year = tslib._localize_pydatetime(next_year, other.tzinfo)
 
-        if other == prev_year:
+        # Note: next_year.year == other.year + 1, so we will always
+        # have other < next_year
+        if norm == prev_year:
             n -= 1
-        elif other == cur_year:
+        elif norm == cur_year:
             pass
-        elif other == next_year:
-            n += 1
-            # TODO: Not hit in tests
         elif n > 0:
-            if other < prev_year:
+            if norm < prev_year:
                 n -= 2
-            elif prev_year < other < cur_year:
+            elif prev_year < norm < cur_year:
                 n -= 1
-            elif cur_year < other < next_year:
+            elif cur_year < norm < next_year:
                 pass
-            else:
-                assert False
         else:
-            if next_year < other:
-                n += 2
-                # TODO: Not hit in tests; UPDATE: looks impossible
-            elif cur_year < other < next_year:
+            if cur_year < norm < next_year:
                 n += 1
-            elif prev_year < other < cur_year:
+            elif prev_year < norm < cur_year:
                 pass
-            elif (other.year == prev_year.year and other < prev_year and
-                  prev_year - other <= timedelta(6)):
+            elif (norm.year == prev_year.year and norm < prev_year and
+                  prev_year - norm <= timedelta(6)):
                 # GH#14774, error when next_year.year == cur_year.year
                 # e.g. prev_year == datetime(2004, 1, 3),
                 # other == datetime(2004, 1, 1)
@@ -1894,35 +1883,30 @@ def apply(self, other):
         return result
 
     def get_year_end(self, dt):
-        if self.variation == "nearest":
-            return self._get_year_end_nearest(dt)
-        else:
-            return self._get_year_end_last(dt)
-
-    def get_target_month_end(self, dt):
-        target_month = datetime(dt.year, self.startingMonth, 1,
-                                tzinfo=dt.tzinfo)
-        return shift_month(target_month, 0, 'end')
-        # TODO: is this DST-safe?
+        assert dt.tzinfo is None
 
-    def _get_year_end_nearest(self, dt):
-        target_date = self.get_target_month_end(dt)
+        dim = ccalendar.get_days_in_month(dt.year, self.startingMonth)
+        target_date = datetime(dt.year, self.startingMonth, dim)
         wkday_diff = self.weekday - target_date.weekday()
         if wkday_diff == 0:
+            # year_end is the same for "last" and "nearest" cases
             return target_date
 
-        days_forward = wkday_diff % 7
-        if days_forward <= 3:
-            # The upcoming self.weekday is closer than the previous one
-            return target_date + timedelta(days_forward)
-        else:
-            # The previous self.weekday is closer than the upcoming one
-            return target_date + timedelta(days_forward - 7)
+        if self.variation == "last":
+            days_forward = (wkday_diff % 7) - 7
 
-    def _get_year_end_last(self, dt):
-        current_year = datetime(dt.year, self.startingMonth, 1,
-                                tzinfo=dt.tzinfo)
-        return current_year + self._offset_lwom
+            # days_forward is always negative, so we always end up
+            # in the same year as dt
+            return target_date + timedelta(days=days_forward)
+        else:
+            # variation == "nearest":
+            days_forward = wkday_diff % 7
+            if days_forward <= 3:
+                # The upcoming self.weekday is closer than the previous one
+                return target_date + timedelta(days_forward)
+            else:
+                # The previous self.weekday is closer than the upcoming one
+                return target_date + timedelta(days_forward - 7)
 
     @property
     def rule_code(self):