Simplify Timedelta init, standardize overflow errors

[patrickmckenna]

• 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.

Per #46936 (comment), this PR extracts the functional updates in #46936, to simplify review. The main motivations are those explained in #46936 (comment):

• 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 it's detected.

As a bonus, this appears to speed things up considerably, especially in the case of passing ints (by avoiding the unnecessary int -> td64[ns] -> int conversion):

       before           after         ratio
     [a853022e]       [d4444404]
     <main>           <td-construction>
-      8.41±0.8μs       7.50±0.2μs     0.89  tslibs.timedelta.TimedeltaConstructor.time_from_unit
-      7.99±0.3μs       4.42±0.1μs     0.55  tslibs.timedelta.TimedeltaConstructor.time_from_string
-     2.80±0.03μs         758±40ns     0.27  tslibs.timedelta.TimedeltaConstructor.time_from_missing
-      6.88±0.3μs       1.67±0.2μs     0.24  tslibs.timedelta.TimedeltaConstructor.time_from_int

I've left out all the test suite updates from #46936, save for those changes associated with s/OverflowError/OutOfBoundsTimedelta.

/cc @jbrockmendel @jreback

[pep8speaks]

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-24 20:22:07 UTC

[patrickmckenna]

I'm a little confused by the errors in the "Minimum Versions" check. (The others errors are also confuse me, but appear to be issues in the CI setup phases.)

The last commit resulted in one less test failure: test_construction_out_of_bounds_td64[3508-M- months] failed for 3c21c7b, but not for f7ee822. (The other parameterizations of that test failed in both commits.) It's unclear to me why that would happen, given this one-line change, or why that test is failing at all—but just for the Minimum Versions check.

Thoughts/feedback here appreciated.

[patrickmckenna]

Update: 9bf564b handles a bizarre string interpolation issue that was coming up in the "Minimum Versions" check. The remaining failures are build issues orthogonal to this PR, and which appear to be fixed in #47015.

[jreback]

minor comments. any performance impact here?

[jreback]

minor comments. any performance impact here?

oh you put that in the top post cool.

then just minor comments.

[jbrockmendel]

any way for less of this logic to be within the try/except?

[patrickmckenna]

Agree, it's not ideal.

Initially, I tried to leave as much conditional processing as possible in convert_to_timedelta64, i.e. no if/else here, but that led to considerable slowdowns in many benchmarks IIRC. So that would be a simpler, albeit slower, option.

A similar option would be to let all the dispatching on arg type happen automatically, and add overflow checking as needed within each variant, e.g.

@functools.singledispatch
def value_to_int(value):
    ...

@value_to_int.register(str)
def _(value: str):
    ...

def create_timedelta(value):
    ...
    try:
        out_value = value_to_int(value)
    catch OverflowError:
        raise OutOfBoundsTimedelta(msg) from err
    ...

I'm not sure if cython plays well with singledispatch? This approach would also expand the scope of this PR a lot.

Definitely open to other ideas though!

[jbrockmendel]

lower case r, also pretty sure this can just be a constant instead of a fixture

[patrickmckenna]

Can switch 👍 (Since this repo includes a devcontainer.json, I thought maybe use R for VS Code (and GitHub) compatibility.)

[github-actions]

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]

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.