Add more overflow tests for timedelta64 operations #46854

patrickmckenna · 2022-04-23T22:50:39Z

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 PR adds more specific tests, to verify the current, sometimes inconsistent behavior. Much of it has been reported/discussed in previous threads, and some appears to reflect intentional tradeoffs, e.g. allowing rounding error to avoid overflows. Other behavior seems likely unintentional:

Inconsistent usage of ValueError and OverflowError
Spurious OverflowErrors for 1-element Series w/ valid values

Since this is my first time poking around pandas/numpy internals, it seemed best to make sure the right tests are in the right places before attempting any fixes.

Open Questions

What exception should be raised if Series.sum() would yield an invalid Timedelta: ValueError, OutOfBoundsTimedelta, something else? (I can rewrite the tests to mark those cases that currently raise the "wrong" exception as xfail for now.)
Is silently introducing rounding error considered ok, or should a warning be emitted?

Is this the right spot for these tests? AFAICT, a lot of this behavior is determined by pandas.core.nanops, particularly special-casing of timedelta64 in nansum():

pandas/pandas/core/nanops.py

Lines 618 to 621 in 4cf8d55

    
           if is_float_dtype(dtype): 
        
               dtype_sum = dtype 
        
           elif is_timedelta64_dtype(dtype): 
        
               dtype_sum = np.dtype(np.float64)

My assumption was that tests against the public API would be preferred, but please LMK if that's not the case.

Prior Art

Currently all timedelta64 sums involve int -> float conversion.

pep8speaks · 2022-04-23T22:50:42Z

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-15 19:13:51 UTC

patrickmckenna

This PR adds some additional overflow tests for timedeltas. It's goal is to accurately capture the current behavior, not fix existing bugs/inconsistencies—but that's the next step 😄

@jbrockmendel as one who's been working on related issues somewhat recently, I'd appreciate your feedback! (Apologies if there are others I should be pinging, too...) This is my first time poking around pandas/numpy internals, so if there's a better way to write/organize the tests, please LMK.

patrickmckenna · 2022-04-25T20:34:50Z

pandas/tests/arithmetic/test_timedelta64.py

 from pandas.tests.arithmetic.common import (
    assert_invalid_addsub_type,
    assert_invalid_comparison,
    get_upcast_box,
 )

+timedelta_types = (Timedelta, TimedeltaArray, TimedeltaIndex, Series, DataFrame)


Most of the newly-added overflow tests are parameterized over Timedelta scalar and container types, on the assumption that similar exceptions should be raised for all. (The fact that they're not can be confusing, as mentioned in #43178.) Is that assumption accurate?

that assumption is accurate

id prefer to keep the Timedelta scalar tests in tests.scalar.timedelta, then use the box_with_array fixture for the non-scalar tests. part of the virtue of this is that when writing the non-scalar tests, we assume that the scalar behavior is correct and tested.

patrickmckenna · 2022-04-25T23:02:35Z

pandas/tests/arithmetic/test_timedelta64.py

+    left = wrap_value(Timedelta.max, add_sub_types.left)
+    right = wrap_value(positive_td, add_sub_types.right)
+
+    if add_sub_types.result is Timedelta:


Should this be raising a OutOfBoundsTimedelta in all cases? Ref: #34448

probably, yes

jreback

thanks for the tests. these look to be nicely comprehensive and you have a lot of questions about these as we may have tradeoffs in the existing operations. If you can move these to a new file will make it easier to run.

pandas/tests/arithmetic/test_timedelta64.py

Some td64 overflow tests remain in other modules: - tests/tslibs/test_conversion.py::test_ensure_timedelta64ns_overflows() - tests/tslibs/test_timedeltas.py::test_huge_nanoseconds_overflow() - tests/scalar/timedelta/test_timedelta.py::test_mul_preserves_reso() - tests/scalar/timedelta/test_constructors.py::test_construct_from_td64_with_unit(),test_overflow_on_construction() Still TBD whether these should remain there or also be migrated. See: github.com/pandas-dev/pull/46854#discussion_r858131625

jbrockmendel · 2022-04-26T18:29:35Z

i'll take a look today

pandas/tests/series/test_reductions.py

pandas/tests/test_timedelta64_overflow.py

patrickmckenna · 2022-04-28T20:36:32Z

pandas/_testing/__init__.py

@@ -279,6 +281,28 @@ def box_expected(expected, box_cls, transpose=True):
    return expected


+def wrap_value(value, cls, transpose=False):


The motivation for this was to simplify writing tests that should produce the same output for both scalar and container types, e.g. here.

are there any other thematically similar functions that could be grouped together in a pandas._testing.foo instead of __init__? we've kind of stagnated, but in principle there's a goal to get stuff out of this file

patrickmckenna · 2022-04-28T20:43:59Z

pandas/tests/arithmetic/test_timedelta64.py

-    timestamp = Timestamp("2021-01-01")
-    result = timestamp + timedelta_range("0s", "1s", periods=31)
-    expected = DatetimeIndex(
+class TestAddSub:


If this organization makes sense, I can open a follow up PR to migrate many of the existing tests into this and similar classes (and make them fully parameterized across all container types).

patrickmckenna · 2022-04-28T20:47:05Z

pandas/tests/test_timedelta64_overflow.py

+    left = wrap_value(Timestamp.min, ts_add_sub_types[1])
+
+    ex = (OutOfBoundsDatetime, OverflowError)
+    msg = "|".join(["Out of bounds nanosecond timestamp", "Overflow in int64 addition"])


Agreed. Imagine this would be part of the same future PR mentioned in #46854 (comment)?

patrickmckenna

@jbrockmendel @jreback per #46854 (comment), I've migrated the new tests back to existing modules. I've also removed the hypothesis usage, and rewritten the tests to follow the pattern of accepting box types as fixtures. Please LMK if you see anything else that needs changing.

Once this ships, I can open up a follow-on PR to make the exception raising more consistent.

patrickmckenna · 2022-04-29T17:48:49Z

pandas/tests/arithmetic/test_timedelta64.py

    )
-    tm.assert_index_equal(result, expected)
+    def test_sub_raises_if_result_would_overflow(


FWIW, I tried rewriting this to keep the test function body constant, and mapping each input value to an appropriate xfail marker:

xfail_does_not_raise = pytest.mark.xfail(reason="doesn't raise", strict=True) xfail_raises_overflow_error = pytest.mark.xfail( reason="raises wrong error", raises=OverflowError, strict=True, ) # ... @pytest.mark.parametrize( "rval", [ pytest.param(Timedelta(1), marks=xfail_does_not_raise), pytest.param(Timedelta(2), marks=xfail_raises_overflow_error), pytest.param(Timedelta.max, marks=xfail_raises_overflow_error), ], ) def test_sub_raises_td64_specific_error_if_result_would_overflow( self, max_td64: TD64_BOX_TYPE, rval: Timedelta, td64_type: Type[TD64_TYPE], ): rvalue = tm.wrap_value(rval, td64_type) min_td64 = -1 * max_td64 with pytest.raises(OutOfBoundsTimedelta, match="too small"): min_td64 - rvalue

Unfortunately that slowed execution by a factor of ~40 (locally). Decorating the test with a single, catch-all xfail marker led to the same results. Passing run=False did help, but it was still ~5x slower.

jbrockmendel · 2022-04-30T01:50:31Z

will take another look tomorrow

jbrockmendel · 2022-05-02T18:19:54Z

pandas/tests/arithmetic/test_timedelta64.py

+from functools import partial
+from typing import (
+    Type,
+    Union,


We can't use type and |?

I wish, that's a 3.10 feature.

even if we do from __future__ import annotations? im sure we use pipes elsewhere

Huh, I'd tried that and switching to pipes, and it was failing for type aliases like TD64_BOX_TYPE = TimedeltaArray | TimedeltaIndex | Series | DataFrame. But TIL: pipes will work for function annotations (with that import).

type does work. I'll switch to that, and replace the aliases with pipes.

jbrockmendel · 2022-05-02T18:23:07Z

pandas/tests/arithmetic/test_timedelta64.py

+    """
+    dt64 = tm.wrap_value(Timestamp.now(), dt64_type)
+    td64 = tm.wrap_value(Timedelta(0), td64_type)
+    return type(dt64 + td64)


so this is going to map Timedelta->Timestamp and TimedeltaArray->DatetimeArray but be an identity mapping for everything else?

I might be misunderstanding the question, but think the gist of what you're suggesting is right (assuming Array stands for every box type). So:

Timedelta + Timestamp -> Timestamp Timedelta + DatetimeArray -> DatetimeArray Timedelta + DatetimeIndex -> DatetimeIndex Timedelta + Series -> Series Timedelta + DataFrame -> DataFrame TimedeltaArray + Timestamp -> DatetimeArray TimedeltaArray + DatetimeArray -> DatetimeArray ... TimedeltaIndex + Timestamp -> DatetimeIndex TimedeltaIndex + DatetimeArray -> DatetimeIndex ...

Happy to update the docstring, or use a different pattern, if that's unclear.

My preference (mentioned #46854 (comment)) is to handle the Timedelta/Timestamp cases in the existing tests/scalar/... files, and the non-scalar tests here. Then this wrapping becomes unnecessary and you can just use box_with_array

How about scalar/box tests, e.g. Timedelta + DatetimeIndex, where should those live?

jreback · 2022-05-04T01:29:08Z

@jbrockmendel happy for you to merge this when you are ready

patrickmckenna · 2022-05-04T01:33:16Z

happy for you to merge this when you are ready

@jreback appreciate that! Alas, no green button for me—I haven't got write access 🙃

jreback · 2022-05-04T01:54:59Z

happy for you to merge this when you are ready

@jreback appreciate that! Alas, no green button for me—I haven't got write access 🙃

@patrickmckenna that was to @jbrockmendel :->

patrickmckenna · 2022-05-04T04:17:44Z

Hah, whoops! 🤦‍♂️😅

patrickmckenna · 2022-05-12T01:48:56Z

Please LMK if there's more that needs doing here. Unsure what to do about the few red x's, which appear to be (transient?) failures during CI setup.

github-actions · 2022-06-15T00:05:59Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

mroeschke · 2022-08-15T16:31:10Z

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

patrickmckenna added 3 commits April 22, 2022 16:46

add tests for existing behavior

f0b34b0

Currently all timedelta64 sums involve int -> float conversion.

note spurious overflow for single elem td64 series

287ca88

finer-grained testing for td64 sum overflow errors

992ca95

styling, ex msg fixes

1ea03e6

patrickmckenna force-pushed the td64-sums branch from 1f8ceaa to 7a35bd2 Compare April 24, 2022 20:24

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

2868200

patrickmckenna force-pushed the td64-sums branch from 7a35bd2 to 2868200 Compare April 24, 2022 20:24

patrickmckenna added 5 commits April 24, 2022 13:43

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

eb1f61b

consolidate, parameterize td64 addition overflow tests

ac91c58

add scalar multiplication tests

b8e4a51

add tests for scalar multiplication

4c72f1e

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

5ef0a48

patrickmckenna changed the title ~~Add more granular tests of current Series[timedelta64[ns]].sum() behavior~~ Add more overflow tests for timedelta64 operations Apr 25, 2022

patrickmckenna added 3 commits April 25, 2022 14:09

mypy, win38 fixes

aeef81c

use box_expected where possible

438339d

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

e86d0df

patrickmckenna commented Apr 25, 2022

View reviewed changes

patrickmckenna marked this pull request as ready for review April 25, 2022 23:21

jreback requested changes Apr 26, 2022

View reviewed changes

pandas/tests/arithmetic/test_timedelta64.py Outdated Show resolved Hide resolved

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

jbrockmendel reviewed Apr 27, 2022

View reviewed changes

pandas/tests/series/test_reductions.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Apr 27, 2022

View reviewed changes

pandas/tests/test_timedelta64_overflow.py Outdated Show resolved Hide resolved