Added ISO 8601 Duration string constructor for Timedelta #19065

WillAyd · 2018-01-03T22:58:39Z

closes Timedelta ISO Format Constructor #19040
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

ASV results below

· Running 16 total benchmarks (2 commits * 1 environments * 8 benchmarks)
[  0.00%] · For pandas commit hash 6eda5188:
[  0.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt...
[  0.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[  6.25%] ··· Running timedelta.TimedeltaConstructor.time_from_components                                                               14.6±0.2μs
[ 12.50%] ··· Running timedelta.TimedeltaConstructor.time_from_datetime_timedelta                                                       7.17±0.1μs
[ 18.75%] ··· Running timedelta.TimedeltaConstructor.time_from_int                                                                     5.86±0.08μs
[ 25.00%] ··· Running timedelta.TimedeltaConstructor.time_from_iso_format                                                               20.7±0.4μs
[ 31.25%] ··· Running timedelta.TimedeltaConstructor.time_from_missing                                                                 2.11±0.03μs
[ 37.50%] ··· Running timedelta.TimedeltaConstructor.time_from_np_timedelta                                                             4.79±0.1μs
[ 43.75%] ··· Running timedelta.TimedeltaConstructor.time_from_string                                                                  6.32±0.08μs
[ 50.00%] ··· Running timedelta.TimedeltaConstructor.time_from_unit                                                                     6.22±0.1μs
[ 50.00%] · For pandas commit hash 6552718d:
[ 50.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt...
[ 50.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 56.25%] ··· Running timedelta.TimedeltaConstructor.time_from_components                                                               14.8±0.2μs
[ 62.50%] ··· Running timedelta.TimedeltaConstructor.time_from_datetime_timedelta                                                      7.28±0.09μs
[ 68.75%] ··· Running timedelta.TimedeltaConstructor.time_from_int                                                                      5.67±0.1μs
[ 75.00%] ··· Running timedelta.TimedeltaConstructor.time_from_iso_format                                                                   failed
[ 81.25%] ··· Running timedelta.TimedeltaConstructor.time_from_missing                                                                 2.09±0.05μs
[ 87.50%] ··· Running timedelta.TimedeltaConstructor.time_from_np_timedelta                                                            4.74±0.04μs
[ 93.75%] ··· Running timedelta.TimedeltaConstructor.time_from_string                                                                   5.97±0.1μs
[100.00%] ··· Running timedelta.TimedeltaConstructor.time_from_unit                                                                     6.29±0.1μs
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

jreback · 2018-01-04T00:06:31Z

pandas/_libs/tslibs/timedeltas.pyx

+    Match a provided string against an ISO 8601 pattern, providing a group for
+    each ``Timedelta`` component.
+    """
+    pater = re.compile(r"""P


you prob want to compile this pattern in a module level variable to avoid repeatedly doing this.

jreback · 2018-01-04T00:07:25Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -506,6 +526,33 @@ def _binary_op_method_timedeltalike(op, name):
 # ----------------------------------------------------------------------
 # Timedelta Construction

+def _value_from_iso_match(match):


make cdef and type this as int64_t

jreback · 2018-01-04T00:07:55Z

pandas/_libs/tslibs/timedeltas.pyx

+    match_dict = {k: int(v) for k, v in match_dict.items()}
+    nano = match_dict.pop('nanoseconds', 0)
+
+    return nano + convert_to_timedelta64(timedelta(**match_dict), 'ns')


just return the nanoseconds here

Not sure I follow - don't we need to the timedelta64 to get the nanosecond precision of all the components?

better to use timedelta_from_spec and construct the number of nanoseconds, then simply return that

jreback · 2018-01-04T00:08:41Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -506,6 +526,33 @@ def _binary_op_method_timedeltalike(op, name):
 # ----------------------------------------------------------------------
 # Timedelta Construction

+def _value_from_iso_match(match):


call this parse_iso_format_string

it shouldn't take the match, but the string itself.

jreback · 2018-01-04T00:09:54Z

pandas/_libs/tslibs/timedeltas.pyx

+                match = match_iso_format(value)
+                value = _value_from_iso_match(match)
+            else:
+                value = np.timedelta64(parse_timedelta_string(value))


this should look like

if len(value) > 0 and vaue[0] == 'P': value = parse_iso_format(value) else: value = parse_timedelta_string(value) value = np.timedelta64(value)

jreback · 2018-01-04T00:10:27Z

pandas/tests/scalar/test_timedelta.py

+        ('P0DT0H0M0.000000123S', Timedelta(nanoseconds=123)),
+        ('P0DT0H0M0.00001S', Timedelta(microseconds=10)),
+        ('P0DT0H0M0.001S', Timedelta(milliseconds=1)),
+        ('P0DT0H1M0S', Timedelta(minutes=1))])


add some invalid matches that should raise (prob need to catch inside the parsing function and raise a nice message)

codecov · 2018-01-04T01:10:44Z

Codecov Report

Merging #19065 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #19065   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files         148      148           
  Lines       48680    48680           
=======================================
  Hits        44550    44550           
  Misses       4130     4130

Flag	Coverage Δ
#multiple	`89.88% <ø> (ø)`	⬆️
#single	`41.68% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35b2aba...6bab9da. Read the comment docs.

jreback

I wonder if a hand written parser is better (though certainly more work), so maybe in the future

jreback · 2018-01-05T00:25:49Z

pandas/_libs/tslibs/timedeltas.pyx

+        for k, v in match_dict.items():
+            ns += timedelta_from_spec(v, '0', k)
+
+    else:


you should just raise here

Not an expert on Cython but pytest was failing when raising the exception inside of this function. I believe it is attributable to the int64_t function declaration. I could remove that type and raising the exception directly inside the function would work. Otherwise, I was getting the below output during testing.

-------------------------------------------------------------- Captured stderr call -------------------------------------------------------------- AttributeError: 'NoneType' object has no attribute 'groupdict' Exception ignored in: 'pandas._libs.tslibs.timedeltas.parse_iso_format_string' AttributeError: 'NoneType' object has no attribute 'groupdict'

oh, you just need to add:
except? -1: to the declaration; this tells cython that you may raise in a cdef function so it should check. you don't need to explicity return -1 though.

Ah OK thanks. Will make changes and re-push

jreback · 2018-01-05T00:26:10Z

pandas/_libs/tslibs/timedeltas.pyx

-            value = np.timedelta64(parse_timedelta_string(value))
+            if len(value) > 0 and value[0] == 'P':
+                iso_val = parse_iso_format_string(value)
+                if iso_val == -1:


this logic is much simpler if you raise above

jreback · 2018-01-05T00:27:11Z

pandas/_libs/tslibs/timedeltas.pyx

+    Returns
+    -------
+    ns: int64_t
+        Precision in nanoseconds of matched ISO 8601 duration, or -1 if


add a Raises section instead

jreback · 2018-01-05T14:13:01Z

thanks!

TomAugspurger · 2018-01-05T14:42:14Z

Just seeing this and #11375 now.

In #11375 (comment) @jorisvandenbossche mentioned these would be better represented as DateOffsets, since there's some debate about whether ISO 8601 durations are relative (DateOffset) or absolute (TimeDelta)

I think it's still useful to support this as a format for parsing timedeltas, but we may want to put some warnings / caveats in the documentation.

WillAyd force-pushed the iso-cons branch from 057daa3 to 0bf7cb4 Compare January 3, 2018 23:27

jreback added the Timedelta Timedelta data type label Jan 4, 2018

jreback requested changes Jan 4, 2018

View reviewed changes

WillAyd force-pushed the iso-cons branch from 0bf7cb4 to 58ebfd3 Compare January 4, 2018 19:14

jreback requested changes Jan 5, 2018

View reviewed changes

WillAyd force-pushed the iso-cons branch from 58ebfd3 to d4df2d6 Compare January 5, 2018 01:24

WillAyd added 2 commits January 4, 2018 17:32

Added ISO 8601 Duration string constructor for Timedelta

e1a09ce

ISO 8601 Duration Refactoring

6bab9da

WillAyd force-pushed the iso-cons branch from d4df2d6 to 6bab9da Compare January 5, 2018 01:32

jreback added this to the 0.23.0 milestone Jan 5, 2018

jreback added the Enhancement label Jan 5, 2018

jreback approved these changes Jan 5, 2018

View reviewed changes

jreback merged commit c6166b0 into pandas-dev:master Jan 5, 2018

WillAyd deleted the iso-cons branch January 5, 2018 14:22

mroeschke mentioned this pull request Jan 9, 2018

CLN: ASV timedelta #19150

Merged

WillAyd mentioned this pull request May 20, 2018

Add Timedelta Support to JSON Reader with orient="table" #21140

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ISO 8601 Duration string constructor for Timedelta #19065

Added ISO 8601 Duration string constructor for Timedelta #19065

WillAyd commented Jan 3, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

WillAyd Jan 4, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

codecov bot commented Jan 4, 2018 •

edited

Loading

jreback left a comment

jreback Jan 5, 2018

WillAyd Jan 5, 2018

jreback Jan 5, 2018

WillAyd Jan 5, 2018

jreback Jan 5, 2018

jreback Jan 5, 2018

jreback commented Jan 5, 2018

TomAugspurger commented Jan 5, 2018

Added ISO 8601 Duration string constructor for Timedelta #19065

Added ISO 8601 Duration string constructor for Timedelta #19065

Conversation

WillAyd commented Jan 3, 2018

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

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2018 • edited Loading

Codecov Report

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

jreback commented Jan 5, 2018

TomAugspurger commented Jan 5, 2018

codecov bot commented Jan 4, 2018 •

edited

Loading