BUG: Breaking change to offsets.pyx - quarter offset classes #39890

Pawel-Kranzberg · 2021-02-18T17:07:42Z

Set default months for BQuarterEnd, BQuarterBegin, QuarterEnd, QuarterBegin classes in order; rename startingMonth parameter to month for those classes.
Addresses #8435 and #5307

closes DEPR: QuarterBegin and BQuarterBegin return days that are not quarter beginnings #8435 Parameter Name of startingMonth for QuarterEnd is Misleading #5307
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Addresses pandas-dev#8435

jreback · 2021-02-18T17:11:59Z

pls just one PR - just push to the same one if changing

Pawel-Kranzberg · 2021-02-18T18:41:27Z

pls just one PR - just push to the same one if changing

But this one has different scope from #39887 . The change regarding months alone in #39887 should hopefully be easier to process, and less breaking.

jreback · 2021-02-18T18:45:07Z

ok that's fine

you always need a test

jreback

you would need to change / add tests to show what is changing. and have the tests pass

jbrockmendel · 2021-02-19T02:22:52Z

does this handle #5307?

Pawel-Kranzberg · 2021-02-19T07:18:38Z

does this handle #5307?

Not at present. I'd like to cover it as well - please let me know if that's OK.

Pawel-Kranzberg · 2021-02-20T14:31:32Z

does this handle #5307?

It does now.

Update test_startingMonth_deprecation

Add quarter-based offsets' changes to the API breaking changes (Notable bug fixes) section

Pawel-Kranzberg · 2021-04-06T14:22:20Z

can you add a sub-section in the whatsnew 1.3 api breaking changes to indicate what a user would except (aside from the deprection)

Description drafted and added.

Pawel-Kranzberg · 2021-04-10T07:07:31Z

so you are adding a new parameter 'month', so now we have the confusing 'startingMonth' and 'month'. I would rather deprecate 'startingMonth', then add new; not even sure 'month' is a good name here. can you write a sample whatsnew note indicate what the change is here from a user POV.

BTW, maybe anchor would be a better replacement name?

jbrockmendel · 2021-04-27T23:19:43Z

doc/source/whatsnew/v1.3.0.rst

+Quarter-based time offset classes :class:`~pandas.tseries.offsets.QuarterEnd`,
+:class:`~pandas.tseries.offsets.QuarterBegin`, :class:`~pandas.tseries.offsets.BQuarterEnd`,
+:class:`~pandas.tseries.offsets.BQuarterBegin` now use ``month`` instead of ``startingMonth``
+as a time anchor parameter (:issue:`5307`). Its default value for :class:`~pandas.tseries.offsets.QuarterBegin`


"Its" -> "The"?

Good catch, fixed.

jbrockmendel · 2021-04-27T23:21:08Z

pandas/_libs/tslibs/offsets.pyx


-    def __init__(self, n=1, normalize=False, startingMonth=None):
+    def __init__(self, n: int=1, normalize: bool=False, month: int=None,
+                 *, startingMonth: int=None):  # GH#5307 backwards compatibility


comment on new line # GH#5307 startingMonth retained for backwards compatibility

Comment added.

jbrockmendel · 2021-04-27T23:22:34Z

pandas/_libs/tslibs/offsets.pyx


-    def __init__(self, n=1, normalize=False, startingMonth=None):
+    def __init__(self, n: int=1, normalize: bool=False, month: int=None,


should we try to detect if month is passed as a non-keyword, in which case the behavior may be silently changing?

(if backward compat werent an issue, I'd be OK with making everything other than n and normalize keyword-only in all these classes)

jbrockmendel · 2021-04-27T23:22:54Z

pandas/_libs/tslibs/offsets.pyx

+            raise TypeError("Offset received both month and startingMonth")
+        elif month is None and startingMonth is not None:
+            warnings.warn(
+                "startingMonth is deprecated, use month instead.",


should this explain that the meaning has changed? or point to a link?

jbrockmendel · 2021-04-27T23:23:07Z

pandas/_libs/tslibs/offsets.pyx

+            warnings.warn(
+                "startingMonth is deprecated, use month instead.",
+                FutureWarning,
+                stacklevel=2


trailing comma, pretend we could run black on this file

Fixed, hopefully.

jreback

@Pawel-Kranzberg can you split this change up. There is just too much going on here.

doc-strings
deprecation
breaking change

I would really like to do the deprecation first and separate

jreback · 2021-05-06T12:32:14Z

doc/source/whatsnew/v1.3.0.rst

@@ -458,6 +458,36 @@ with a :class:`MultiIndex` in the result. This can lead to a perceived duplicati
    df.groupby('label1').rolling(1).sum()


+.. _whatsnew_130.notable_bug_fixes.quarterbegin_default_anchor:
+
+Changed default periods in :class:`~pandas.tseries.offsets.QuarterBegin` and :class:`~pandas.tseries.offsets.BQuarterBegin`


this needs to be the same length as the title, it should error if not

jreback · 2021-05-06T12:32:59Z

doc/source/whatsnew/v1.3.0.rst

+
+*pandas 1.3.0*
+
+.. code-block:: ipython


don't use a code-block, rather an ipython block for the new behavior

jreback · 2021-05-06T12:34:14Z

doc/source/whatsnew/v1.3.0.rst

@@ -458,6 +458,36 @@ with a :class:`MultiIndex` in the result. This can lead to a perceived duplicati
    df.groupby('label1').rolling(1).sum()


+.. _whatsnew_130.notable_bug_fixes.quarterbegin_default_anchor:


you need to say deprecation about the startingMonth as its mentioned below as well. I would prefer making these 2 distinct and unrelated changes (which they are). so don't mention deprecation at all here.

jreback · 2021-05-06T12:35:15Z

doc/source/whatsnew/v1.3.0.rst

@@ -625,6 +656,7 @@ Datetimelike
 - Bug in :meth:`Timedelta.round`, :meth:`Timedelta.floor`, :meth:`Timedelta.ceil` for values near the implementation bounds of :class:`Timedelta` (:issue:`38964`)
 - Bug in :func:`date_range` incorrectly creating :class:`DatetimeIndex` containing ``NaT`` instead of raising ``OutOfBoundsDatetime`` in corner cases (:issue:`24124`)
 - Bug in :func:`infer_freq` incorrectly fails to infer 'H' frequency of :class:`DatetimeIndex` if the latter has a timezone and crosses DST boundaries (:issue:`39556`)
+- Bug in offsets :class:`QuarterBegin` and :class:`BQuarterBegin` returning days that are not conventional quarter beginnings (:issue:`8435`)


isn't this a duplicate of the above section?

jreback · 2021-05-06T12:37:31Z

pandas/_libs/tslibs/offsets.pyx

-            startingMonth = self._default_starting_month
-        self.startingMonth = startingMonth
+        # GH#5307 startingMonth retained for backwards compatibility
+        if month and startingMonth and month != startingMonth:


don't you need to check not-none? e.g. 0 is value that would hit this too (which would fail but is misleading no?)

jreback · 2021-05-06T12:37:59Z

pandas/_libs/tslibs/offsets.pyx

-        self.startingMonth = state.pop("startingMonth")
+        try:
+            self.month = state.pop("month")
+        except:


yeah this is non-obvious on the exception.

github-actions · 2021-06-09T00:03:13Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

simonjayhawkins · 2021-06-16T13:23:09Z

I would really like to do the deprecation first and separate

@Pawel-Kranzberg what's the status. close this and open separate PRs?

Pawel-Kranzberg · 2021-06-17T13:27:20Z

I would really like to do the deprecation first and separate

@Pawel-Kranzberg what's the status. close this and open separate PRs?

Yes, I plan to do so next week.

mroeschke · 2021-08-17T00:30:49Z

Thanks for the PR, but it has appeared to have gone stale. Closing due to inactivity, but we would be happy to take a new PR working on the deprecation first.

Breaking change to offsets.pyx

d4428da

Addresses pandas-dev#8435

jreback requested changes Feb 19, 2021

View reviewed changes

jreback added the Frequency DateOffsets label Feb 19, 2021

Pawel-Kranzberg added 6 commits February 20, 2021 13:22

Update offsets.pyx

a546ddc

Update offsets.pyx

d564763

Update offsets.pyx

85ee259

Update offsets.pyx

8b4ed54

Update offsets.pyx

6e1f3c5

Update offsets.pyx

01ce09f

Pawel-Kranzberg added 15 commits February 20, 2021 16:37

Update test_constructors.py

b95cc70

Update test_misc.py

ccb9b97

Update test_period_index.py

f1204de

Update offsets.pyx

a8c8a3d

Update test_dst.py

1338c1f

Update test_dst.py

d7c8cad

Update test_offsets.py

3399d67

Update test_offsets.py

79fe747

Update test_constructors.py

2f456cb

Update test_misc.py

3fd9a64

Update test_offsets.py

3ce1de9

Update test_yqm_offsets.py

7e1e5d2

Update test_period_index.py

f9e7c3b

Update test_offsets.py

27a24ba

Update test_offsets.py

197fa1b

Pawel-Kranzberg added 10 commits April 5, 2021 21:33

Update test_offsets.pyx according to GH#39890 review

448e0da

Update test_startingMonth_deprecation

Update offsets.pyx

fe07ea2

Update offsets.pyx

5b2ec3f

Update v1.3.0.rst

827ab65

Add quarter-based offsets' changes to the API breaking changes (Notable bug fixes) section

Update offsets.pyx

f27b5ef

Update v1.3.0.rst

1441a29

Update offset_frequency.rst

7875614

Update v0.18.0.rst

688eaf6

Update v1.3.0.rst

c0f6d84

Update v0.18.0.rst

a5416a1

Pawel-Kranzberg requested a review from jreback April 6, 2021 14:22

Merge branch 'master' into patch-2

1d3d45d

Pawel-Kranzberg requested a review from jbrockmendel April 27, 2021 06:47

jbrockmendel reviewed Apr 27, 2021

View reviewed changes

Pawel-Kranzberg added 2 commits May 6, 2021 09:32

Update v1.3.0.rst

22c779f

Update offsets.pyx

384cb82

jreback requested changes May 6, 2021

View reviewed changes

github-actions bot added the Stale label Jun 9, 2021

Pawel-Kranzberg marked this pull request as draft June 10, 2021 17:02

mroeschke closed this Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Breaking change to offsets.pyx - quarter offset classes #39890

BUG: Breaking change to offsets.pyx - quarter offset classes #39890

Pawel-Kranzberg commented Feb 18, 2021 •

edited

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021 •

edited

jreback commented Feb 18, 2021

jreback left a comment

jbrockmendel commented Feb 19, 2021

Pawel-Kranzberg commented Feb 19, 2021

Pawel-Kranzberg commented Feb 20, 2021

Pawel-Kranzberg commented Apr 6, 2021

Pawel-Kranzberg commented Apr 10, 2021

jbrockmendel Apr 27, 2021

Pawel-Kranzberg May 6, 2021

jbrockmendel Apr 27, 2021

Pawel-Kranzberg May 6, 2021

jbrockmendel Apr 27, 2021

jbrockmendel Apr 27, 2021

jbrockmendel Apr 27, 2021

Pawel-Kranzberg May 6, 2021

jreback left a comment

jreback May 6, 2021

jreback May 6, 2021

jreback May 6, 2021

jreback May 6, 2021

jreback May 6, 2021

jreback May 6, 2021

github-actions bot commented Jun 9, 2021

simonjayhawkins commented Jun 16, 2021

Pawel-Kranzberg commented Jun 17, 2021

mroeschke commented Aug 17, 2021


		def __init__(self, n=1, normalize=False, startingMonth=None):
		def __init__(self, n: int=1, normalize: bool=False, month: int=None,

		@@ -458,6 +458,36 @@ with a :class:`MultiIndex` in the result. This can lead to a perceived duplicati
		df.groupby('label1').rolling(1).sum()


		.. _whatsnew_130.notable_bug_fixes.quarterbegin_default_anchor:

BUG: Breaking change to offsets.pyx - quarter offset classes #39890

BUG: Breaking change to offsets.pyx - quarter offset classes #39890

Conversation

Pawel-Kranzberg commented Feb 18, 2021 • edited

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021 • edited

jreback commented Feb 18, 2021

jreback left a comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 19, 2021

Pawel-Kranzberg commented Feb 19, 2021

Pawel-Kranzberg commented Feb 20, 2021

Pawel-Kranzberg commented Apr 6, 2021

Pawel-Kranzberg commented Apr 10, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 9, 2021

simonjayhawkins commented Jun 16, 2021

Pawel-Kranzberg commented Jun 17, 2021

mroeschke commented Aug 17, 2021

Pawel-Kranzberg commented Feb 18, 2021 •

edited

Pawel-Kranzberg commented Feb 18, 2021 •

edited