Fix FY5253 onOffset/apply bug, simplify #18877

jbrockmendel · 2017-12-20T17:59:07Z

Similar to #18875.

The actual bugfix here is just changing the comparison in apply from other < whatever to norm < whatever. The remaining edits to get_year_end are orthogonal simplification (which I guess could go in a separate PR).

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jbrockmendel · 2017-12-20T17:59:54Z

pandas/tseries/offsets.py

                pass
-            else:
-                assert False


Double-checked this, it cannot be reached. See new comment above re next_year.year.

jbrockmendel · 2017-12-20T18:00:00Z

pandas/tseries/offsets.py

        else:
-            if next_year < other:
-                n += 2
-                # TODO: Not hit in tests; UPDATE: looks impossible


Double-checked this, it cannot be reached. See new comment above re next_year.year.

jbrockmendel · 2017-12-20T18:00:11Z

pandas/tseries/offsets.py

            pass
-        elif other == next_year:
-            n += 1
-            # TODO: Not hit in tests


Double-checked this, it cannot be reached. See new comment above re next_year.year.

jbrockmendel · 2017-12-20T18:01:23Z

pandas/tseries/offsets.py

-        target_month = datetime(dt.year, self.startingMonth, 1,
-                                tzinfo=dt.tzinfo)
-        return shift_month(target_month, 0, 'end')
-        # TODO: is this DST-safe?


non-issue b/c get_target_month_end is only ever called with dt.tzinfo is None

jbrockmendel · 2017-12-20T18:01:52Z

pandas/tseries/offsets.py

-
-    def _get_year_end_last(self, dt):
-        current_year = datetime(dt.year, self.startingMonth, 1,
-                                tzinfo=dt.tzinfo)


only called with dt.tzinfo is None

jbrockmendel · 2017-12-20T18:02:32Z

That should be the last new one for a while... I hope.

jbrockmendel · 2017-12-20T18:36:24Z

Close was accidental.

jreback · 2017-12-21T14:16:24Z

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(


are these tests wrong?

No, but get_target_month_end is removed in this PR.

jreback · 2017-12-21T14:17:02Z

pandas/tests/tseries/offsets/test_fiscal.py

+
+
+def test_fy5253_last_onoffset():
+    offset = FY5253(n=-5, startingMonth=5, variation="last", weekday=0)


add a comment to these tests about what you are testing. next person to come along will have no idea. add the issue number as well.

jreback · 2017-12-21T14:18:54Z

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`)


can you give a typical case here, so a user would know to look closer about this.

…fsets-fy5253

codecov · 2017-12-22T03:36:32Z

Codecov Report

Merging #18877 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18877      +/-   ##
==========================================
- Coverage   91.59%   91.57%   -0.02%     
==========================================
  Files         150      150              
  Lines       48924    48911      -13     
==========================================
- Hits        44813    44792      -21     
- Misses       4111     4119       +8

Flag	Coverage Δ
#multiple	`89.94% <100%> (-0.02%)`	⬇️
#single	`41.17% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`97.07% <100%> (+0.28%)`	⬆️
pandas/plotting/_converter.py	`65.22% <0%> (-1.74%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80a5399...9f02521. Read the comment docs.

…fsets-fy5253

jreback

minor formatting. ping on green.

jreback · 2017-12-23T20:10:45Z

pandas/tseries/offsets.py

@@ -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


pls add a blank line (generally) before comments.

jreback · 2017-12-23T20:11:19Z

pandas/tseries/offsets.py

-            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


jreback · 2017-12-23T20:11:43Z

pandas/tseries/offsets.py

+            # variation == "nearest":
+            days_forward = wkday_diff % 7
+            if days_forward <= 3:
+                # The upcoming self.weekday is closer than the previous one


these comments are good, FYI (you only need a blank line when the indentation is the same)

…fsets-fy5253

jbrockmendel · 2017-12-24T13:44:03Z

Ping

jbrockmendel · 2017-12-27T02:52:33Z

This is a blocker for addressing bugs in FY5253Quarter.

jreback · 2017-12-27T20:23:21Z

lgtm. conflict in whatsnew. ping when pushed / green.

…fsets-fy5253

jbrockmendel · 2017-12-28T03:21:18Z

ping

jreback · 2017-12-28T12:28:47Z

thanks!

simplify nearest/last, fix comparison bug

7f2bcf4

jbrockmendel commented Dec 20, 2017

View reviewed changes

jbrockmendel closed this Dec 20, 2017

jbrockmendel reopened this Dec 20, 2017

jbrockmendel mentioned this pull request Dec 20, 2017

Implement roll_monthday, simplify SemiMonthOffset #18762

Merged

4 tasks

jreback requested changes Dec 21, 2017

View reviewed changes

jreback added the Frequency DateOffsets label Dec 21, 2017

jreback requested changes Dec 21, 2017

View reviewed changes

jreback added the Bug label Dec 21, 2017

jbrockmendel added 2 commits December 21, 2017 09:11

more informative whatsnew, comments for tests

ed7fde0

Merge branch 'master' of https://github.com/pandas-dev/pandas into of…

e0850fa

…fsets-fy5253

Merge branch 'master' of https://github.com/pandas-dev/pandas into of…

81b1805

…fsets-fy5253

jbrockmendel mentioned this pull request Dec 23, 2017

BUG: fix Series[timedelta64] arithmetic with Timedelta scalars #18831

Merged

4 tasks

jreback requested changes Dec 23, 2017

View reviewed changes

jreback added this to the 0.23.0 milestone Dec 23, 2017

jbrockmendel added 2 commits December 23, 2017 14:09

Merge branch 'master' of https://github.com/pandas-dev/pandas into of…

a9a5cdc

…fsets-fy5253

requested whitespace for comments

e08a1a7

jreback approved these changes Dec 27, 2017

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into of…

9f02521

…fsets-fy5253

jreback merged commit f42ae78 into pandas-dev:master Dec 28, 2017

jbrockmendel deleted the offsets-fy5253 branch February 11, 2018 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FY5253 onOffset/apply bug, simplify #18877

Fix FY5253 onOffset/apply bug, simplify #18877

jbrockmendel commented Dec 20, 2017

jbrockmendel Dec 20, 2017

jbrockmendel Dec 20, 2017

jbrockmendel Dec 20, 2017

jbrockmendel Dec 20, 2017 •

edited

Loading

jbrockmendel Dec 20, 2017

jbrockmendel commented Dec 20, 2017

jbrockmendel commented Dec 20, 2017

jreback Dec 21, 2017

jbrockmendel Dec 21, 2017

jreback Dec 21, 2017

jreback Dec 21, 2017

codecov bot commented Dec 22, 2017 •

edited

Loading

jreback left a comment

jreback Dec 23, 2017

jreback Dec 23, 2017

jreback Dec 23, 2017

jbrockmendel commented Dec 24, 2017

jbrockmendel commented Dec 27, 2017

jreback commented Dec 27, 2017

jbrockmendel commented Dec 28, 2017

jreback commented Dec 28, 2017



		def test_fy5253_last_onoffset():
		offset = FY5253(n=-5, startingMonth=5, variation="last", weekday=0)

Fix FY5253 onOffset/apply bug, simplify #18877

Fix FY5253 onOffset/apply bug, simplify #18877

Conversation

jbrockmendel commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Dec 20, 2017

jbrockmendel commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 22, 2017 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Dec 24, 2017

jbrockmendel commented Dec 27, 2017

jreback commented Dec 27, 2017

jbrockmendel commented Dec 28, 2017

jreback commented Dec 28, 2017

jbrockmendel Dec 20, 2017 •

edited

Loading

codecov bot commented Dec 22, 2017 •

edited

Loading