Cleaning yearly, quarterly and monthly date offsets #39878

Pawel-Kranzberg · 2021-02-18T00:18:33Z

Ability to use YearBegin and QuarterBegin DateOffset classes as Period freq, along with offset aliases.
Ability to use three-letter month short names as month / startingMonth arguments of year- / quarter-based offset classes (as already used in their _from_name class method).
Expanded docstrings with, e.g., additional examples.
Partial refactorization, as a step toward combining QuarterOffset and YearOffset __init__ (mentioned as TO DO in the code).
xref ENH: Implement '_period_dtype_code' attribute to 'pandas._libs.tslibs.offsets.MonthBegin' #38859
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Fixes to year, quarter and month offsets.

jreback · 2021-02-18T00:45:31Z

@Pawel-Kranzberg you don't need multiple PR
pls use the same one and just push new commits

jbrockmendel · 2021-02-18T01:52:51Z

pandas/_libs/tslibs/offsets.pyx

-
-    cdef readonly:
-        int month
+    cdef readonly int month


we generally use the patternw tih only a single cdef

cdef: public int _period_dtype_code readonly int month [...]

@jbrockmendel - Thanks, I've just fixed that.
The problem with the failing tests stems from the change of the default month for BQuarterBegin and QuarterBegin classes, from 3 (March) to 1 (January). I believe the old values to be wrong, leading to behaviours that users might not expect, and inconsistency vs. BQuarterEnd and QuarterEnd. I've for instance expected the first quarter to begin with January and end in March (that's already the case for BQuarterEnd and QuarterEnd), and not begin in March and end in May (the legacy BQuarterBegin and QuarterBegin do just that).
However, the current tests expect the legacy (IMO incorrect) quarter spans. How should I go about it?

Minor formatting changes.

Temporary unfixing of the default months for `QuarterBegin` and `BQuarterBegin`.

jreback · 2021-02-18T10:19:44Z

if you are just refactoring then all tests should pass

please do any breaking change after this PR - it's very hard to tell what you are doing if these are conflated

Pawel-Kranzberg · 2021-02-18T14:42:02Z

@jreback - It's hard for me to do just refactoring when the final test is failing due to the class QuarterBegin having different default values depending on the creation method. See:

cdef class QuarterBegin(QuarterOffset):
    """
    DateOffset increments between Quarter start dates.
    startingMonth = 1 corresponds to dates like 1/01/2007, 4/01/2007, ...
    startingMonth = 2 corresponds to dates like 2/01/2007, 5/01/2007, ...
    startingMonth = 3 corresponds to dates like 3/01/2007, 6/01/2007, ...
    """
    _default_starting_month = 3
    _from_name_starting_month = 1
    _prefix = "QS"
    _day_opt = "start"

The test is as follows:

        sdate = datetime(1999, 12, 25)
        edate = datetime(2000, 1, 1)

        idx1 = date_range(start=sdate, end=edate, freq="QS")
        idx2 = date_range(
            start=sdate, end=edate, freq=offsets.QuarterBegin(startingMonth=1)
        )
>       assert len(idx1) == len(idx2)

Please advise me how to proceed. Should I, e.g., now create and process a new PR with only the default month changes? Or roll back refactoring of Quarter classes in the current PR?

jreback · 2021-02-18T14:47:44Z

pls don't do refactoring and a bug fix in the same PR
one or the other or this will be impossible to merge

jreback · 2021-02-18T14:48:02Z

be honest i don't know which bug this is supposedly fixing

Pawel-Kranzberg · 2021-02-18T15:02:04Z

be honest i don't know which bug this is supposedly fixing

@jreback - OK, I'd say it is a valid bug that a QuarterBegin object has different starting months, depending on if it was created using:

the offset alias. e.g., in date_range(start=sdate, end=edate, freq="QS") , with the resulting quarter having January as the starting month (_from_name_starting_month = 1), or
directly, e.g. in date_range(start=sdate, end=edate, freq=offsets.QuarterBegin(), with March as the starting month (_default_starting_month = 3).

In addition I'd argue that default starting months for QuarterBegin and BQuarterBegin should be 1 instead of the current 3.

Pawel-Kranzberg · 2021-02-18T15:06:29Z

Other breaking changes that I introduced allow using year, quarter and month starts, as well as business quarter and month starts, as freq in date_range.

jreback · 2021-02-18T15:17:51Z

pls search the tracker pretty sure there are existing issues for this

we like targeted PRs
PRa that do too many things won't be merged

Pawel-Kranzberg · 2021-02-18T22:40:19Z

@jreback - Thank you for all the guidance and patience. This PR is now narrowed down to the following:

Ability to use all year- and quarter-based offset classes as Period freq, along with offset aliases.
Ability to use three-letter month short names as month / startingMonth arguments of year- / quarter-based offset classes (as already used in their _from_name class method).
Expanded docstrings with, e.g., additional examples.
Partial refactorization, as a step toward combining QuarterOffset and YearOffset __init__ (mentioned as TO DO in the code).

Is that scope acceptable?

jreback

can you add some tests for construction from the str & anything else that is an enhancement. I am not exactly sure where this is useful. maybe some tests would show. Please also craft a whatsnew that shows from a user POV what has changed.

jreback · 2021-02-19T01:33:53Z

pandas/_libs/tslibs/offsets.pyx

+    Timestamp('2021-01-01 05:01:15')
+    >>> ts + YearBegin(normalize = True)
+    Timestamp('2021-01-01 00:00:00')
+    >>> ts + YearBegin()._from_name('APR')


i wouldn't show internal methods

jreback · 2021-02-19T01:34:01Z

pandas/_libs/tslibs/offsets.pyx

-    Timestamp('2020-11-30 05:01:15')
+    >>> ts + YearEnd(normalize = True)
+    Timestamp('2020-12-31 00:00:00')
+    >>> ts + YearEnd()._from_name('AUG')


jreback · 2021-02-21T18:19:20Z

@Pawel-Kranzberg I am still not sure what this PR is doing. seems you updated some doc-string & added some new fucntionaility which is not tested.

jbrockmendel · 2021-02-21T20:06:14Z

pandas/_libs/tslibs/offsets.pyx

-        int month
+    cdef:
+        readonly int month, _period_dtype_code
+        # public int _period_dtype_code


pls dont leave commented-out code

jbrockmendel · 2021-02-21T20:06:30Z

pandas/_libs/tslibs/offsets.pyx


-    # _default_month: int  # FIXME: python annotation here breaks things
+    _attributes = tuple(["n", "normalize", "month"])
+    _default_month = 1


if this cant be annotated, then please leave the comment

jbrockmendel · 2021-02-21T20:07:44Z

pandas/_libs/tslibs/offsets.pyx

-        self.month = month
+        if month is not None:
+            if isinstance(month, str):
+                self.month = MONTH_TO_CAL_NUM[month]


this is adding support for passing a month string? thats not necessarily a bad idea, but would need to check perf impact

jbrockmendel · 2021-02-21T20:08:28Z

pandas/_libs/tslibs/offsets.pyx

@@ -1903,30 +1913,44 @@ cdef class YearOffset(SingleConstructorOffset):
        return shifted


-cdef class BYearEnd(YearOffset):
+cdef class YearBegin(YearOffset):


are you re-ordering the classes in this file? or is git just mangling the diff?

jbrockmendel · 2021-02-21T20:09:18Z

pls start by reverting the re-ordering of classes. that will make it much easier to see what is actually changed

jbrockmendel · 2021-03-14T00:58:50Z

@Pawel-Kranzberg can you address comments and merge master

Pawel-Kranzberg · 2021-03-15T07:58:50Z

@Pawel-Kranzberg can you address comments and merge master

Yes, I will. I'm sorry, I've been very busy lately.

Sync with master

github-actions · 2021-04-30T00:10:16Z

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.

Pawel-Kranzberg · 2021-04-30T12:14:31Z

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.

I need it for now as a reference.

simonjayhawkins · 2021-06-16T13:27:04Z

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.

I need it for now as a reference.

you can still reference closed PRs. will close this one as you have several open that are stale. ping when ready to continue and will reopen.

Pawel-Kranzberg added 7 commits February 17, 2021 17:39

Update offsets.pyx

a944a27

Fixes to year, quarter and month offsets.

Merge remote-tracking branch 'upstream/master'

fb439fc

Update offsets.pyx

87e1b87

Update offsets.pyx

5d46697

Update offsets.pyx

a583905

Update offsets.pyx

cd216ae

Update offsets.pyx

53b09c1

jbrockmendel reviewed Feb 18, 2021

View reviewed changes

Pawel-Kranzberg added 7 commits February 18, 2021 07:51

Update offsets.pyx

6fdb0af

Minor formatting changes.

Update offsets.pyx

b7d671b

Minor formatting changes.

Add ability to provide months as short names

de72073

Update offsets.pyx

bfaf6ce

Update offsets.pyx

1b396dd

Update offsets.pyx

d7f7020

Update offsets.pyx

ca67f8a

Temporary unfixing of the default months for `QuarterBegin` and `BQuarterBegin`.

Pawel-Kranzberg added 6 commits February 18, 2021 11:58

Update offsets.pyx

e04e27d

Update offsets.pyx

d4d87c4

Update offsets.pyx

2c22a31

Update offsets.pyx

11cb0b5

Update offsets.pyx

71e9cab

Update offsets.pyx

29edef3

Update offsets.pyx

0fa3644

jreback requested changes Feb 19, 2021

View reviewed changes

jreback added Frequency DateOffsets Refactor Internal refactoring of code labels Feb 19, 2021

Pawel-Kranzberg marked this pull request as draft February 20, 2021 09:37

Pawel-Kranzberg added 10 commits February 20, 2021 19:45

Update offsets.pyx

818cbdf

Update offsets.pyx

1d14b0f

Update offsets.pyx

700ba0e

Update offsets.pyx

648c7a8

Update offsets.pyx

fdc7e54

Update offsets.pyx

070cd72

Update offsets.pyx

efb314e

Update offsets.pyx

045177e

Update offsets.pyx

4f15b5b

Update offsets.pyx

fbf3f68

jbrockmendel reviewed Feb 21, 2021

View reviewed changes

Pawel-Kranzberg added 2 commits March 25, 2021 21:00

Merge remote-tracking branch 'upstream/master'

6c53c39

Merge pull request #2 from pandas-dev/master

6f90cb3

Sync with master

github-actions bot added the Stale label Apr 30, 2021

simonjayhawkins closed this Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaning yearly, quarterly and monthly date offsets #39878

Cleaning yearly, quarterly and monthly date offsets #39878

Pawel-Kranzberg commented Feb 18, 2021 •

edited by jreback

Loading

jreback commented Feb 18, 2021

jbrockmendel Feb 18, 2021

Pawel-Kranzberg Feb 18, 2021 •

edited

Loading

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021

jreback commented Feb 18, 2021

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021 •

edited

Loading

jreback left a comment •

edited

Loading

jreback Feb 19, 2021

jreback Feb 19, 2021

jreback commented Feb 21, 2021

jbrockmendel Feb 21, 2021

jbrockmendel Feb 21, 2021

jbrockmendel Feb 21, 2021

jbrockmendel Feb 21, 2021

jbrockmendel commented Feb 21, 2021

jbrockmendel commented Mar 14, 2021

Pawel-Kranzberg commented Mar 15, 2021

github-actions bot commented Apr 30, 2021

Pawel-Kranzberg commented Apr 30, 2021

simonjayhawkins commented Jun 16, 2021

Cleaning yearly, quarterly and monthly date offsets #39878

Cleaning yearly, quarterly and monthly date offsets #39878

Conversation

Pawel-Kranzberg commented Feb 18, 2021 • edited by jreback Loading

jreback commented Feb 18, 2021

jbrockmendel Feb 18, 2021

Choose a reason for hiding this comment

Pawel-Kranzberg Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021

jreback commented Feb 18, 2021

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021

jreback commented Feb 18, 2021

Pawel-Kranzberg commented Feb 18, 2021 • edited Loading

jreback left a comment • edited Loading

Choose a reason for hiding this comment

jreback Feb 19, 2021

Choose a reason for hiding this comment

jreback Feb 19, 2021

Choose a reason for hiding this comment

jreback commented Feb 21, 2021

jbrockmendel Feb 21, 2021

Choose a reason for hiding this comment

jbrockmendel Feb 21, 2021

Choose a reason for hiding this comment

jbrockmendel Feb 21, 2021

Choose a reason for hiding this comment

jbrockmendel Feb 21, 2021

Choose a reason for hiding this comment

jbrockmendel commented Feb 21, 2021

jbrockmendel commented Mar 14, 2021

Pawel-Kranzberg commented Mar 15, 2021

github-actions bot commented Apr 30, 2021

Pawel-Kranzberg commented Apr 30, 2021

simonjayhawkins commented Jun 16, 2021

Pawel-Kranzberg commented Feb 18, 2021 •

edited by jreback

Loading

Pawel-Kranzberg Feb 18, 2021 •

edited

Loading

Pawel-Kranzberg commented Feb 18, 2021 •

edited

Loading

jreback left a comment •

edited

Loading