Add/reorganize scalar Timedelta tests #46936

patrickmckenna · 2022-05-04T01:02:22Z

closes #xxxx (Replace xxxx with the Github issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

This is a companion of sorts to #46854. It adds more thorough tests, and groups them by type, but doesn't change any behavior.

/cc @jbrockmendel @jreback

pandas/tests/scalar/timedelta/test_constructors.py

This is a quick and dirty v0 approach.

pep8speaks · 2022-05-04T22:55:05Z

Hello @patrickmckenna! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-17 01:55:27 UTC

pandas/_libs/tslibs/timedeltas.pyx

jbrockmendel · 2022-05-10T22:50:01Z

it looks like this PR has evolved to involve a lot of stuff. i'd suggest breaking it into tightly-scoped pieces for easier (faster) review

pandas/_libs/ops.pyx

pandas/_libs/tslibs/timedeltas.pyx

patrickmckenna · 2022-05-10T23:35:38Z

it looks like this PR has evolved to involve a lot of stuff. i'd suggest breaking it into tightly-scoped pieces for easier (faster) review

Yes, it's comically large at the moment, apologies. Fully understand and agree it needs to be smaller to make review not a PITA. (I'd been keeping it open, as a draft, to simplify running the full test suite as I iterated on a few ideas.)

I'll push a few more changes and add some high-level comments shortly, to figure out how best to split this up.

patrickmckenna

The overwhelming majority of line changes come from edits to 2 test files:

pandas/tests/scalar/timedelta/test_arithmetic.py
pandas/tests/scalar/timedelta/test_timedelta.py

I've tried to keep all existing tests, especially those that map to specific PRs, albeit sometimes simplified, or updated to test a broader range of inputs. I also removed pandas/tests/scalar/timedelta/test_{constructors,formats.py}, as they contained a lot of duplicate code, and moved the unique stuff to the other 2 files mentioned above.

The other component of this PR is a few updates to Timedelta, simplifying/deduping the constructor logic and performing overflow checks. I can extract those bits into a standalone PR, though that would still leave a ton of changed lines to review.

patrickmckenna · 2022-05-10T23:52:20Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -892,6 +882,42 @@ cdef _timedelta_from_value_and_reso(int64_t value, NPY_DATETIMEUNIT reso):
    return td_base


+@cython.overflowcheck(True)
+cdef object create_timedelta(object value, str in_unit, NPY_DATETIMEUNIT out_reso):


This is one of the main functional changes introduced by this PR as it stands. The motivations:

DRY up Timedelta creation logic, which was divided and duplicated over several functions

Do so outside Timdelta.__new__, since (AFAICT) there shouldn't yet be a public API for creating instances with non-nano resolution

Add overflow checks, and consistently raise OutOfBoundsTimedelta when its detected.

I can move these changes to a separate PR, to keep this one focused on tests alone, if that's desired.

patrickmckenna · 2022-05-10T23:57:14Z

pandas/_libs/tslibs/timedeltas.pyx

        if value is _no_input:
-            if not len(kwargs):


The simplifications here are of a piece with the introduction of create_timedelta above. Timedelta.__new__ had duplicated much of the logic in convert_to_timedelta64, so that's removed. It's now mostly a wrapper around create_timedelta, which wraps convert_to_timedelta64 and _timedelta_from_value_and_reso.

patrickmckenna · 2022-05-10T23:57:39Z

pandas/_libs/tslibs/timedeltas.pyx

-                + int(kwargs.get('microseconds', 0) * 1_000)
-                + int(kwargs.get('milliseconds', 0) * 1_000_000)
-                + seconds
+                    kwargs["weeks"] * 7 * 24 * 3600 * 1_000_000_000,


This should really be extracted into a separate function, but that's for later.

patrickmckenna · 2022-05-11T00:12:53Z

pandas/tests/scalar/timedelta/test_arithmetic.py

@@ -1,34 +1,159 @@
 """


The main changes, both here and in test_timedelta.py:

Use fixtures to extract, simplify repeated test setup

Split up tests with many asserts, and/or distinct sections of setup code, into separate tests.

Parameterize tests, wherever sensible, over all arithmetic ops of a similar kind, e.g. add and radd

Parameterize tests to run with Array/Index/Series/DataFrames.

Consistent grouping of like tests into classes

More descriptive test names

patrickmckenna · 2022-05-11T00:15:30Z

pandas/tests/scalar/timedelta/test_constructors.py

@@ -1,394 +0,0 @@
-from datetime import timedelta


This file largely duplicated a subset of the tests in test_timedelta.py.

patrickmckenna · 2022-05-11T00:16:41Z

pandas/tests/scalar/timedelta/test_timedelta.py

@@ -1,5 +1,19 @@
-""" test the scalar Timedelta """
+"""


Comments are always at risk of becoming outdated or misleading, but hopefully a few brief notes here are helpful.

patrickmckenna · 2022-05-11T00:18:05Z

pandas/tests/scalar/timedelta/test_timedelta.py

    Timedelta,
    TimedeltaIndex,
    offsets,
    to_timedelta,
 )
 import pandas._testing as tm

+TD_UNITS = (


Some, but not all, of these seem to be defined in other modules, though they're not always exposed for testing, e.g.

pandas/pandas/_libs/tslibs/timedeltas.pyx

Line 95 in 1bc2741

cdef dict timedelta_abbrevs = {

patrickmckenna · 2022-05-11T02:41:41Z

It's a tale of two cities on the perf front. A local run just now:

       before           after         ratio
     [c22bc653]       [99f927ee]
     <td64-tests^2>       <td64-tests>
+      2.97±0.2μs      4.66±0.08μs     1.57  tslibs.timedelta.TimedeltaConstructor.time_from_missing
+     7.85±0.05μs         11.5±2μs     1.47  tslibs.timedelta.TimedeltaConstructor.time_from_string
+     3.59±0.06μs      5.18±0.06μs     1.44  tslibs.timedelta.TimedeltaConstructor.time_from_np_timedelta
-        8.78±1μs      7.17±0.09μs     0.82  tslibs.timedelta.TimedeltaConstructor.time_from_unit
-      8.54±0.8μs      5.66±0.03μs     0.66  tslibs.timedelta.TimedeltaConstructor.time_from_components
-      6.87±0.5μs      1.57±0.02μs     0.23  tslibs.timedelta.TimedeltaConstructor.time_from_int

Looking into the regressions.

patrickmckenna · 2022-05-11T18:20:18Z

Looking into the regressions.

Better now:

> asv continuous -f 1.1 upstream/main HEAD -b tslibs.timedelta.TimedeltaConstructor
...
       before           after         ratio
     [a853022e]       [d0967da6]
                      <tmp/td64-perf>
-        15.4±2μs       11.2±0.7μs     0.73  tslibs.timedelta.TimedeltaConstructor.time_from_iso_format
-      8.72±0.8μs       5.75±0.2μs     0.66  tslibs.timedelta.TimedeltaConstructor.time_from_components
-      8.20±0.4μs       5.06±0.3μs     0.62  tslibs.timedelta.TimedeltaConstructor.time_from_string
-      6.52±0.1μs       1.60±0.2μs     0.25  tslibs.timedelta.TimedeltaConstructor.time_from_int
-      3.24±0.3μs          744±6ns     0.23  tslibs.timedelta.TimedeltaConstructor.time_from_missing

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

patrickmckenna · 2022-05-11T20:24:52Z

it looks like this PR has evolved to involve a lot of stuff. i'd suggest breaking it into tightly-scoped pieces for easier (faster) review

Yes, it's comically large at the moment, apologies. Fully understand and agree it needs to be smaller to make review not a PITA. (I'd been keeping it open, as a draft, to simplify running the full test suite as I iterated on a few ideas.)

I'll push a few more changes and add some high-level comments shortly, to figure out how best to split this up.

I've extracted the functional updates, which touch many fewer lines, into #47004, so this PR can focus on de-duping/expanding test coverage.

patrickmckenna · 2022-05-17T22:41:55Z

Once #46854 and #47004 merge, I can drastically cut the scope of this PR, and focus it on just consolidating/deduping scalar Timedelta tests. In particular, my thought is to move everything from timedelta/test_{constructors,formats}.py to timedelta/test_timedelta.py, to avoid having related yet incomplete test cases spread across three modules.

If that's not useful, or isn't worth prioritizing, please LMK.

jbrockmendel · 2022-05-22T03:29:01Z

If that's not useful, or isn't worth prioritizing, please LMK.

honestly im having trouble motivating myself to carefully review this, but i'll give it another go once the other PRs are done. id advise against spending much time here in the interim

patrickmckenna · 2022-05-25T01:05:05Z

Think it might be easier to just close this for now. If there are elements worth reviewing and merging, they can always be cherry-picked into a much more narrowly-scoped PR. Or this can be reopened.

(As mentioned, I left this open in its current form only to simplify checking CI results especially the Windows and 32-bit runs. But it's probably too big now to easily whittle down into something reasonable and useful.)

add more Timedelta tests

aa47af9

patrickmckenna commented May 4, 2022

View reviewed changes

pandas/tests/scalar/timedelta/test_constructors.py Show resolved Hide resolved

pandas/tests/scalar/timedelta/test_constructors.py Show resolved Hide resolved

pandas/tests/scalar/timedelta/test_constructors.py Outdated Show resolved Hide resolved

patrickmckenna marked this pull request as ready for review May 4, 2022 01:09

whoops, toolz is optional

0957c27

patrickmckenna marked this pull request as draft May 4, 2022 17:35

patrickmckenna added 4 commits May 4, 2022 10:36

s/OverflowError/OutOfBoundsTimedelta where relevant

89b2431

This is a quick and dirty v0 approach.

catch correct error in Timedelta sub

51eb151

extract overflow-checking wrapper fcn, add tests

36d37dc

Merge remote-tracking branch 'upstream/main' into td-constructor

ef09bb9

patrickmckenna added 4 commits May 4, 2022 17:21

simplify Timedelta ctor, fix tests

d53d1ce

Merge remote-tracking branch 'upstream/main' into td64-tests

e788c25

test, linter fixes

d995243

Merge remote-tracking branch 'upstream/main' into td64-tests

43291f8

patrickmckenna force-pushed the td64-tests branch from aa376c5 to 43291f8 Compare May 5, 2022 04:28

add wrapper for overflow-checked int/float ops

4247286

jreback added Testing pandas testing functions or related to the test suite Timedelta Timedelta data type labels May 6, 2022

patrickmckenna added 2 commits May 6, 2022 15:58

wip: lots more tests, consolidate overflow checking

33203b3

Merge remote-tracking branch 'upstream/main' into td64-tests

1e73327

patrickmckenna force-pushed the td64-tests branch from 64837c4 to 1e73327 Compare May 6, 2022 23:01

patrickmckenna added 3 commits May 6, 2022 18:12

more test cleanup

a72d4e0

handle older and newer cython semantics

7a7418a

misc. small fixes

b5e62d6

patrickmckenna force-pushed the td64-tests branch from bc3abed to b5e62d6 Compare May 8, 2022 05:29

DRY up tests

42e597c

patrickmckenna force-pushed the td64-tests branch from bae55bb to 42e597c Compare May 9, 2022 17:27

patrickmckenna added 3 commits May 10, 2022 10:05

Merge remote-tracking branch 'upstream/main' into td64-tests

36eb26f

consolidate Timedelta creation

c9e209e

remove unused stuff

4a73340

jbrockmendel reviewed May 10, 2022

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed May 10, 2022

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx Show resolved Hide resolved

jbrockmendel reviewed May 10, 2022

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed May 10, 2022

View reviewed changes

pandas/_libs/ops.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed May 10, 2022

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx Outdated Show resolved Hide resolved

describe current organization, PR feedback

1bc2741

patrickmckenna force-pushed the td64-tests branch from 8510414 to 1bc2741 Compare May 10, 2022 23:46

patrickmckenna commented May 11, 2022

View reviewed changes

patrickmckenna added 2 commits May 10, 2022 18:42

use preferred names

f00539f

Merge remote-tracking branch 'upstream/main' into td64-tests

99f927e

perf optimizations

d0967da

patrickmckenna added a commit to patrickmckenna/pandas that referenced this pull request May 11, 2022

extract functional updates from pandas-dev#46936

4b58319

patrickmckenna mentioned this pull request May 11, 2022

Simplify Timedelta init, standardize overflow errors #47004

Closed

4 tasks

patrickmckenna added 2 commits May 15, 2022 14:46

Merge remote-tracking branch 'upstream/main' into td64-tests

0dbf13d

Merge remote-tracking branch 'upstream/main' into td64-tests

a913282

patrickmckenna mentioned this pull request May 17, 2022

BUG: Timedelta resolution is different depending on how the argument is passed #33992

Open

3 tasks

patrickmckenna closed this May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/reorganize scalar Timedelta tests #46936

Add/reorganize scalar Timedelta tests #46936

patrickmckenna commented May 4, 2022 •

edited

Loading

pep8speaks commented May 4, 2022 •

edited

Loading

jbrockmendel commented May 10, 2022

patrickmckenna commented May 10, 2022

patrickmckenna left a comment

patrickmckenna May 10, 2022

patrickmckenna May 10, 2022

patrickmckenna May 10, 2022

patrickmckenna May 11, 2022

patrickmckenna May 11, 2022

patrickmckenna May 11, 2022

patrickmckenna May 11, 2022

patrickmckenna commented May 11, 2022

patrickmckenna commented May 11, 2022 •

edited

Loading

patrickmckenna commented May 11, 2022

patrickmckenna commented May 17, 2022

jbrockmendel commented May 22, 2022

patrickmckenna commented May 25, 2022

Add/reorganize scalar Timedelta tests #46936

Add/reorganize scalar Timedelta tests #46936

Conversation

patrickmckenna commented May 4, 2022 • edited Loading

pep8speaks commented May 4, 2022 • edited Loading

Comment last updated at 2022-05-17 01:55:27 UTC

jbrockmendel commented May 10, 2022

patrickmckenna commented May 10, 2022

patrickmckenna left a comment

Choose a reason for hiding this comment

patrickmckenna May 10, 2022

Choose a reason for hiding this comment

patrickmckenna May 10, 2022

Choose a reason for hiding this comment

patrickmckenna May 10, 2022

Choose a reason for hiding this comment

patrickmckenna May 11, 2022

Choose a reason for hiding this comment

patrickmckenna May 11, 2022

Choose a reason for hiding this comment

patrickmckenna May 11, 2022

Choose a reason for hiding this comment

patrickmckenna May 11, 2022

Choose a reason for hiding this comment

patrickmckenna commented May 11, 2022

patrickmckenna commented May 11, 2022 • edited Loading

patrickmckenna commented May 11, 2022

patrickmckenna commented May 17, 2022

jbrockmendel commented May 22, 2022

patrickmckenna commented May 25, 2022

patrickmckenna commented May 4, 2022 •

edited

Loading

pep8speaks commented May 4, 2022 •

edited

Loading

patrickmckenna commented May 11, 2022 •

edited

Loading