BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH41356) #41488

jackzyliu · 2021-05-15T15:20:04Z

closes BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset #41356 CustomBusinessMonthBegin offset parameter #14869
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Problem description: _CustomBusinessMonth does not apply extra offset when initially rolled month begin new is already a business day. This happens because we currently use CustomBusinessDay.rollforward (rollback) to both 1) roll to a business day and 2) apply the extra offset.
Fix: Separating the logic to 1) roll to a business day and 2) apply the extra offset.

pandas/_libs/tslibs/offsets.pyx

jbrockmendel · 2021-05-17T04:47:14Z

doc/source/whatsnew/v1.3.0.rst

@@ -971,6 +971,7 @@ Other
 - Bug in :func:`pandas.util.show_versions` where console JSON output was not proper JSON (:issue:`39701`)
 - Bug in :meth:`DataFrame.convert_dtypes` incorrectly raised ValueError when called on an empty DataFrame (:issue:`40393`)
 - Bug in :meth:`DataFrame.clip` not interpreting missing values as no threshold (:issue:`40420`)
+- Bug in :meth:`_CustomBusinessMonth.apply` not applying extra offset when initially rolled date `new` is already a business day (:issue:`41356`)


_CustomBusinessMonth is not user-facing. can this be phrased in terms of something users will directly see?

Thanks for the feedback! I just re-phased the entry. Is it more along the line of what you were thinking?

jreback · 2021-05-17T15:10:31Z

pandas/tests/tseries/offsets/test_month.py

+                datetime(2021, 3, 2): datetime(2021, 3, 30) + timedelta(days=1),
+            },
+        ),
+    ]


can you just put these directly in the paremeterize?

Thanks for the review! Will do.

As a side note, the existing tests for .apply also put the case definition outside of parametrize . Happy to quickly change these as well if you would like.

jreback · 2021-05-17T15:10:42Z

pandas/tests/tseries/offsets/test_month.py

+        ),
+    ]
+
+    @pytest.mark.parametrize("case", apply_with_extra_offset_cases)


jreback · 2021-05-17T15:12:02Z

pandas/_libs/tslibs/offsets.pyx

@@ -3350,7 +3350,13 @@ cdef class _CustomBusinessMonth(BusinessMixin):
        """
        Define default roll function to be called in apply method.
        """
-        cbday = CustomBusinessDay(n=self.n, normalize=False, **self.kwds)
+        if self.offset:


isn't offset always defined?

Yes, the purpose of the check here is to make sure we only pass in a zero self.offset into CustomBusinessDay (and that we do not do unnecessary copying of the kwd dict).

pandas/_libs/tslibs/offsets.pyx

jbrockmendel · 2021-05-18T19:40:10Z

doc/source/whatsnew/v1.3.0.rst

@@ -974,6 +974,7 @@ Other
 - Bug in :meth:`DataFrame.convert_dtypes` incorrectly raised ValueError when called on an empty DataFrame (:issue:`40393`)
 - Bug in :meth:`DataFrame.clip` not interpreting missing values as no threshold (:issue:`40420`)
 - Bug in :class:`Series` backed by :class:`DatetimeArray` or :class:`TimedeltaArray` sometimes failing to set the array's ``freq`` to ``None`` (:issue:`41425`)
+- Bug in :meth:`CustomBusinessMonthBegin` (:meth:`CustomBusinessMonthEnd`) not applying the extra ``offset`` parameter when beginning (end) of the target month is already a business day (:issue:`41356`)


you've given the class but not the method. Is it e.g. CustomBusinessMonthBegin.__add__?

Oops, my apologies. It should be corrected now. I mentioned the function .apply to be more specific, but maybe I should say .__add__ to be more user-centric?

simonjayhawkins · 2021-05-25T10:31:29Z

@jackzyliu can you merge master to resolve conflicts

jackzyliu · 2021-05-25T20:08:23Z

Hi @simonjayhawkins, I just rebased after master, but some checks started failing and I can't replicate the failed tests offline. Examples:

pandas/tests/tseries/offsets/test_offsets_properties.py::test_on_offset_implementations with actions-38.yaml
pandas/tests/arithmetic/test_datetime64.py::TestDatetimeIndexArithmetic::test_sub_dti_dti with actions-39-numpydev.yaml

These checks were successful before rebasing without any code change. What could be the issue here?

simonjayhawkins · 2021-05-26T11:28:13Z

The test_on_offset_implementations failure is a hypothesis failure so that maybe the reason it didn't fail before. may need to investigate that one further. The numpydev failures are a known issue.

jackzyliu · 2021-06-09T04:14:50Z

Re-rebased after master. @simonjayhawkins

The test_on_offset_implementations failure is a hypothesis failure so that maybe the reason it didn't fail before. may need to investigate that one further. The numpydev failures are a known issue.

Previously, pandas/tests/tseries/offsets/test_offsets_properties.py::test_on_offset_implementations unrelatedly failed with the following pair of randomly generated test inputs,:

dt = datetime.datetime(1900, 1, 1, 0, 0, tzinfo=pytz.timezone('Africa/Kinshasa'))
offset = pd.offsets.MonthBegin(66)

I created a separate issue and pull request for this.

jackzyliu · 2021-07-06T06:13:47Z

Bumping up this PR for further review. Since we just released v1.3.0, I have moved the whatsnew entry to 1.3.1 for now. Happy to move it to 1.4.0 instead.

The failed test seem to come from master?

Please let me know of further comments. Thanks.

jackzyliu · 2021-07-14T06:02:36Z

@jreback @jbrockmendel @simonjayhawkins

Moved whatsnew entry to 1.4.0
Rebased after master
All checks green

Please let me know of further comments. Thanks!

…41356) CustomBusinessMonthBegin(End) does not apply extra offset when the initially rolled month begin (end) is already a business day.

jackzyliu · 2021-07-24T21:35:29Z

Resolved conflict with master. Green.

jreback · 2021-07-28T01:50:22Z

thanks @jackzyliu, sorry about the wait, lots of PRs :->

keep em coming!

…41356) (pandas-dev#41488)

jackzyliu commented May 15, 2021

View reviewed changes

pandas/_libs/tslibs/offsets.pyx Show resolved Hide resolved

jackzyliu mentioned this pull request May 15, 2021

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset #41356

Closed

3 tasks

jbrockmendel added the Frequency DateOffsets label May 17, 2021

jbrockmendel reviewed May 17, 2021

View reviewed changes

jreback requested changes May 17, 2021

View reviewed changes

jackzyliu force-pushed the test_custom_b_month_begin branch from 32ce31b to bc6d718 Compare May 18, 2021 19:12

jbrockmendel reviewed May 18, 2021

View reviewed changes

jackzyliu force-pushed the test_custom_b_month_begin branch from bc6d718 to 52bfd15 Compare May 18, 2021 19:57

simonjayhawkins added this to the 1.3 milestone May 25, 2021

jackzyliu force-pushed the test_custom_b_month_begin branch from 52bfd15 to 02f6ceb Compare May 25, 2021 18:38

simonjayhawkins removed this from the 1.3 milestone May 26, 2021

jackzyliu force-pushed the test_custom_b_month_begin branch 2 times, most recently from 7068952 to 8cb2f65 Compare June 9, 2021 02:12

jackzyliu mentioned this pull request Jun 9, 2021

BUG: pytz.AmbiguousTimeError not caught in hypothesis test_on_offset_implementations #41906

Closed

3 tasks

jackzyliu force-pushed the test_custom_b_month_begin branch from 8cb2f65 to 7b9a061 Compare June 16, 2021 04:49

jackzyliu force-pushed the test_custom_b_month_begin branch from 7b9a061 to 54c7070 Compare July 6, 2021 05:09

jackzyliu force-pushed the test_custom_b_month_begin branch 2 times, most recently from 4a8fcec to ba151ef Compare July 14, 2021 04:58

jackzyliu force-pushed the test_custom_b_month_begin branch from ba151ef to 47bf257 Compare July 23, 2021 19:16

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH…

dcc2143

…41356) CustomBusinessMonthBegin(End) does not apply extra offset when the initially rolled month begin (end) is already a business day.

jackzyliu force-pushed the test_custom_b_month_begin branch 2 times, most recently from de9c975 to b85ba8d Compare July 24, 2021 02:54

whatsnew entry (GH41356)

05c8ee8

jackzyliu force-pushed the test_custom_b_month_begin branch from b85ba8d to 05c8ee8 Compare July 24, 2021 20:40

jreback added this to the 1.4 milestone Jul 28, 2021

jreback approved these changes Jul 28, 2021

View reviewed changes

jreback merged commit 8e79e33 into pandas-dev:master Jul 28, 2021

CGe0516 pushed a commit to CGe0516/pandas that referenced this pull request Jul 29, 2021

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH…

c04ba31

…41356) (pandas-dev#41488)

feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH…

03cecf3

…41356) (pandas-dev#41488)

mroeschke mentioned this pull request Oct 31, 2021

CustomBusinessMonthBegin offset parameter #14869

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH41356) #41488

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH41356) #41488

jackzyliu commented May 15, 2021 •

edited

Loading

jbrockmendel May 17, 2021

jackzyliu May 18, 2021

jreback May 17, 2021

jackzyliu May 17, 2021

jreback May 17, 2021

jreback May 17, 2021

jackzyliu May 17, 2021

jbrockmendel May 18, 2021

jackzyliu May 18, 2021 •

edited

Loading

simonjayhawkins commented May 25, 2021

jackzyliu commented May 25, 2021

simonjayhawkins commented May 26, 2021

jackzyliu commented Jun 9, 2021 •

edited

Loading

jackzyliu commented Jul 6, 2021 •

edited

Loading

jackzyliu commented Jul 14, 2021

jackzyliu commented Jul 24, 2021

jreback commented Jul 28, 2021

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH41356) #41488

BUG: CustomBusinessMonthBegin(End) sometimes ignores extra offset (GH41356) #41488

Conversation

jackzyliu commented May 15, 2021 • edited Loading

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

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

jackzyliu May 18, 2021 • edited Loading

Choose a reason for hiding this comment

simonjayhawkins commented May 25, 2021

jackzyliu commented May 25, 2021

simonjayhawkins commented May 26, 2021

jackzyliu commented Jun 9, 2021 • edited Loading

jackzyliu commented Jul 6, 2021 • edited Loading

jackzyliu commented Jul 14, 2021

jackzyliu commented Jul 24, 2021

jreback commented Jul 28, 2021

jackzyliu commented May 15, 2021 •

edited

Loading

jackzyliu May 18, 2021 •

edited

Loading

jackzyliu commented Jun 9, 2021 •

edited

Loading

jackzyliu commented Jul 6, 2021 •

edited

Loading