From 7f2bcf4f36ce40b9bfcabffff16fef915ebef0f6 Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Wed, 20 Dec 2017 09:55:34 -0800 Subject: [PATCH 1/3] simplify nearest/last, fix comparison bug --- doc/source/whatsnew/v0.22.0.txt | 1 + pandas/tests/tseries/offsets/test_fiscal.py | 28 +++++--- pandas/tseries/offsets.py | 77 ++++++++------------- 3 files changed, 48 insertions(+), 58 deletions(-) diff --git a/doc/source/whatsnew/v0.22.0.txt b/doc/source/whatsnew/v0.22.0.txt index 0579a80aad28e..eb25119381f89 100644 --- a/doc/source/whatsnew/v0.22.0.txt +++ b/doc/source/whatsnew/v0.22.0.txt @@ -354,3 +354,4 @@ Other - Improved error message when attempting to use a Python keyword as an identifier in a ``numexpr`` backed query (:issue:`18221`) - Bug in :class:`Timestamp` where comparison with an array of ``Timestamp`` objects would result in a ``RecursionError`` (:issue:`15183`) +- Bug in :class:`FY5253` where ``datetime`` addition and subtraction incremented incorrectly in some corner cases (:issue:`18854`) diff --git a/pandas/tests/tseries/offsets/test_fiscal.py b/pandas/tests/tseries/offsets/test_fiscal.py index 2dd061dcc6f9e..9692e008465f5 100644 --- a/pandas/tests/tseries/offsets/test_fiscal.py +++ b/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,20 @@ 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(): + 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(): + 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 diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py index 8b12b2f3ad2ce..37802050cb9d5 100644 --- a/pandas/tseries/offsets.py +++ b/pandas/tseries/offsets.py @@ -1783,13 +1783,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 @@ -1810,6 +1803,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)) @@ -1821,33 +1816,27 @@ def apply(self, other): prev_year = tslib._localize_pydatetime(prev_year, other.tzinfo) cur_year = tslib._localize_pydatetime(cur_year, other.tzinfo) next_year = tslib._localize_pydatetime(next_year, other.tzinfo) + # Note: next_year.year == other.year + 1, so we will always + # have other < next_year - if other == prev_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) @@ -1863,35 +1852,29 @@ 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) + assert dt.tzinfo is None - 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? - - 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) + if self.variation == "last": + days_forward = (wkday_diff % 7) - 7 + # days_forward is always negative, so we always end up + # in the same year as dt + return target_date + timedelta(days=days_forward) else: - # The previous self.weekday is closer than the upcoming one - return target_date + timedelta(days_forward - 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 + # 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): From ed7fde00171e771a8c7c4e31633968f0187516fc Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Thu, 21 Dec 2017 09:11:48 -0800 Subject: [PATCH 2/3] more informative whatsnew, comments for tests --- doc/source/whatsnew/v0.22.0.txt | 2 +- pandas/tests/tseries/offsets/test_fiscal.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v0.22.0.txt b/doc/source/whatsnew/v0.22.0.txt index eb25119381f89..98dbf5c332dd9 100644 --- a/doc/source/whatsnew/v0.22.0.txt +++ b/doc/source/whatsnew/v0.22.0.txt @@ -354,4 +354,4 @@ Other - Improved error message when attempting to use a Python keyword as an identifier in a ``numexpr`` backed query (:issue:`18221`) - Bug in :class:`Timestamp` where comparison with an array of ``Timestamp`` objects would result in a ``RecursionError`` (:issue:`15183`) -- Bug in :class:`FY5253` where ``datetime`` addition and subtraction incremented incorrectly in some corner cases (:issue:`18854`) +- Bug in :class:`FY5253` where ``datetime`` addition and subtraction incremented incorrectly for dates on the year-end but not normalized to midnight (:issue:`18854`) diff --git a/pandas/tests/tseries/offsets/test_fiscal.py b/pandas/tests/tseries/offsets/test_fiscal.py index 9692e008465f5..09206439e9996 100644 --- a/pandas/tests/tseries/offsets/test_fiscal.py +++ b/pandas/tests/tseries/offsets/test_fiscal.py @@ -617,6 +617,7 @@ def test_bunched_yearends(): 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') @@ -626,6 +627,7 @@ def test_fy5253_last_onoffset(): 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) From e08a1a7c30f81e476962d491d8b8aa5f29440da0 Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Sat, 23 Dec 2017 14:11:05 -0800 Subject: [PATCH 3/3] requested whitespace for comments --- pandas/tseries/offsets.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py index 1e052f7243c77..0e6a2259274ed 100644 --- a/pandas/tseries/offsets.py +++ b/pandas/tseries/offsets.py @@ -1847,9 +1847,9 @@ def apply(self, other): prev_year = tslib._localize_pydatetime(prev_year, other.tzinfo) cur_year = tslib._localize_pydatetime(cur_year, other.tzinfo) next_year = tslib._localize_pydatetime(next_year, other.tzinfo) + # Note: next_year.year == other.year + 1, so we will always # have other < next_year - if norm == prev_year: n -= 1 elif norm == cur_year: @@ -1894,6 +1894,7 @@ def get_year_end(self, dt): if self.variation == "last": days_forward = (wkday_diff % 7) - 7 + # days_forward is always negative, so we always end up # in the same year as dt return target_date + timedelta(days=days_forward)