-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: Add isocalendar accessor to DatetimeIndex and Series.dt #33220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Add isocalendar accessor to DatetimeIndex and Series.dt #33220
Conversation
6463c51
to
18b7fef
Compare
Thanks for the PR. I agree the naming is a little weird, though do we need a dedicated property for this? Might be more natural to just convert to the appropriate period and still use the .year attribute therefrom |
Agreed with @WillAyd. Since |
@WillAyd @mroeschke Thanks for the comments! I agree that the Regarding whether this functionality is needed at all, I think if |
I'd say if we include Generally, I believe we should align all |
Yes, I think a series of tuples is not completely user friendly. What about like what's done in
.dt.isocalendar.year
.dt.isocalendar.week
.dt.isocalendar.day What do you think? Re the deprecation of |
+1 to that API @mgmarino. Interested in working on both the |
Definitely! I'll need a bit of a hand with the Deprecation process, but I think I'm pretty clear how to implement For the |
Sure sounds good.
Definitely. Feel free to retool this PR (and original issue) about adding |
18b7fef
to
dc02967
Compare
bd86caf
to
90cb3c1
Compare
FYI, the failing tests seem to be due to visual studio not liking the ctuple return. I’ll still need some time to investigate a fix and/or rewrite to avoid the ctuple. |
Ok, I'm pretty confident that the build will now succeed (it succeeded on my local Azure pipelines), but here's the news:
So, I would understand if this feature is not important enough to do this, so I'm happy to have your feedback, if I should:
Thanks for your help on this! |
I think it's okay to bump Cython since it's a build dependency cc @jbrockmendel. |
a0bff50
to
0bff61c
Compare
Current comments addressed, failing doc build seems to be handled with #33309. |
6f41853
to
60d7088
Compare
i think we're OK to bump cython to the newest bugfix release |
pandas/core/indexes/accessors.py
Outdated
|
||
Examples | ||
-------- | ||
>>> pd.to_datetime(pd.Series(["2020-01-01"])).dt.isocalendar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just create one datetime Series
in the beginning and just reference that in the other examples:
>>> ser = pd.Series(pd.to_datetime(["2020-01-01", "NaT"]))
>>> ser
...
>>> ser.dt.isocalendar
>>> ser.dt.isocalendar.week
6064812
to
9c30351
Compare
Rebased, happy to hear any further comments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments. can you add some prose in timeseries.rst about this (basically just showing it, add a versionadded 1.1 tag). ping on green.
pandas/core/arrays/datetimes.py
Outdated
|
||
sarray = fields.build_isocalendar_sarray(self.asi8) | ||
iso_calendar_df = DataFrame( | ||
sarray, columns=["year", "week", "day"], dtype="Int64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use UInt32 (as that's the default for all calendar component ops)
pandas/_libs/tslibs/fields.pyx
Outdated
with nogil: | ||
for i in range(count): | ||
if dtindex[i] == NPY_NAT: | ||
ret_val = -1, -1, -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make these return 0
pandas/core/indexes/accessors.py
Outdated
>>> ser.dt.isocalendar.week | ||
0 53 | ||
1 <NA> | ||
Name: week, dtype: Int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will need to update this as per the dtype change
@jreback Thanks for the comments, I finally understood it now. Dtype -> UInt32, Doc added, Tests green. |
lgtm. cc @TomAugspurger ok here? |
this lgtm. let's merge on green. |
Thanks @mgmarino! Very nice. Happy to have a follow up to deprecate weekofyear. |
Since |
Good point @jorisvandenbossche. @mgmarino mind creating a follow up PR that makes |
For consistency with `Timestamp.isocalendar`, this should rather be a method. Followup of pandas-dev#33220, see the discussions following the merge
@mroeschke Sure no problem, seems reasonable. The PR is open, I'll ping on green.
Will do soon! |
For consistency with `Timestamp.isocalendar`, this should rather be a method. Followup of pandas-dev#33220, see the discussions following the merge
For consistency with `Timestamp.isocalendar`, this should rather be a method. Followup of #33220, see the discussions following the merge
For consistency with `Timestamp.isocalendar`, this should rather be a method. Followup of pandas-dev#33220, see the discussions following the merge
This PR adds the the isocalendar property to
DatetimeIndex
and the correspondingSeries.dt
accessor. The property returns a DataFrame, e.g.:and
The behavior is consistent with
Timestamp.isocalendar
anddatetime.date.isocalendar
.For more information about ISO 8601 calendar, see e.g. https://en.wikipedia.org/wiki/ISO_week_date.
Address GH33206
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Note that I am very happy to rename the field, but I wanted to go ahead and open up the PR.