-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: Timedelta isoformat #15136
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: Timedelta isoformat #15136
Conversation
fyi the parsing issue is here: #11375 |
""" | ||
Format Timedelta as ISO 8601 Duration like | ||
`P[n]Y[n]M[n]DT[n]H[n]M[n]S`, where the `[n]`s are replaced by the | ||
values for that duration. |
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.
maybe add the link to the 8601 format ?
`ISO8601 duration`_ | ||
|
||
.. _ISO601 duration: https://en.wikipedia.org/wiki/ISO_8601 | ||
|
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.
this link in the doc string as well
lgtm minor did comment |
result = td.isoformat() | ||
expected = 'P0DT0H0M0.000000123S' | ||
self.assertEqual(result, expected) | ||
|
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.
add a test for NaT (which i think should just be nan), similar to what we do for Timestamp
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.
Good call. How about NaT
to match Timestamp
?
In [24]: pd.Timestamp('NaT').isoformat()
Out[24]: 'NaT'
In [25]: pd.Timedelta('NaT').isoformat()
Out[25]: 'NaT'
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.
yep that's fine
d9192f4
to
4605da1
Compare
Thanks for the review. Added the |
Current coverage is 86.23% (diff: 100%)@@ master #15136 diff @@
==========================================
Files 145 145
Lines 51352 54034 +2682
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 43932 46598 +2666
- Misses 7420 7436 +16
Partials 0 0
|
4605da1
to
bab2953
Compare
lgtm. merge on pass. |
Do we need to note that a |
@jorisvandenbossche does a @pwalsh does that sound right to you? |
bab2953
to
28f86b4
Compare
Yeah, it is not really clear from the wiki page. It depends on how the 'days' are interpreted, but from "But keep in mind that "PT36H" is not the same as "P1DT12H" when switching from or to Daylight saving time." I would conclude that the days are also relative and not absolute (not 1D == 24H), and I think for a timedelta this is absolute? |
28f86b4
to
e13c8d4
Compare
@jorisvandenbossche rereading the wiki page again today. I missed this section earlier
How do you read that? I'm going back and forth on it. I think that means we're OK, as python uses time deltas to represent the time between two points, and the spec allows
As one valid representation. I am concerned that other libraries don't implement an |
|
Not in the sense that it is defined on the wiki (or at least the possible notations for a duration). For example, "1 month" is not representable by a Timedelta in general. But anyway, this is probably too detailed discussion for this use case as long as we only specify days and below. |
e13c8d4
to
9fda713
Compare
@TomAugspurger my only remaining question is this in the JSON standard anywhere / common practice? (or is there anything for describing how to store durations / intervals)? For datetimes we have ISO and as epochs, is there such a thing for durations? (if so maybe we should have a kw arg |
|
ok @TomAugspurger then I think your |
merge on pass. |
thanks @TomAugspurger |
Author: Tom Augspurger <[email protected]> Closes pandas-dev#15136 from TomAugspurger/timedelta-isoformat and squashes the following commits: 9fda713 [Tom Augspurger] ENH: Timedelta isoformat
Split from #14904
A few questions before I finish all the docs / tests:
The spec seems very permissive about what's allowed, so we need to choose
(P0Y0M...)
(no)P0DT...
vs.PT...
(yes)...T5H
vsT05H
(no)...6.000000000S
vs.6S
(yes)We probably want a reader for this format as well, maybe in the Timedelta constructor. Not sure if I'll have time to get to that before 0.20 . I'd rather prioritize the JSON table schema PR.