Port Timedelta implementation to tslibs.timedeltas
State: closed

Cut/paste Timedelta and _Timedelta methods into tslibs.timesdeltas._Timedelta.  The moved methods are copied verbatim.

Timedelta._binary_op_method_timedeltalike and Timedelta._op_unary_method do not use `self`, are moved out of the class definitions (but stay in tslib).  (In a follow-up _op_unary_method can be moved to tslibs.timedeltas, _binary_op_method_timedeltalike cannot.)

Most of the rest of _Timedelta&Timedelta can be moved into tslibs.timedeltas._Timedelta after #17793.

On the side, cleanup in core.indexes of redundant isinstance checks.

- [ ] closes #xxxx
- [ ] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Internals
description: Related to non-user accessible pandas implementation
name: Timedelta
description: Timedelta data type
name: Clean

Comments:
is this mostly cut/paste?
> is this mostly cut/paste?

Yes.  Some methods in `Timedelta` are moved into the `cdef`d `_Timedelta`, but their implementations are unchanged.  `Timedelta._op_unary_method` and `Timedelta._binary_op_method_timedeltalike` are made module-level functions instead of methods, but their implementations are unchanged.

The only actual changes are in `core.indexes`, where `isinstance(foo, (datetime, Timestamp))` is changed to `isinstance(foo, datetime)`, ditto for `(timedelta, Timedelta)` --> `timedelta`.

There are some non-trivial upcoming changes, but I'm trying to make your life easier by not mixing those in with cut/paste edits.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=h1) Report
> Merging [#17937](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/15fa4bd62c5c7ac7ac618a7e68742b7eb1f90d21?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17937/graphs/tree.svg?src=pr&token=eZ4WkYLtcO&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17937      +/-   ##
==========================================
- Coverage   91.25%   91.23%   -0.02%     
==========================================
  Files         163      163              
  Lines       50120    50120              
==========================================
- Hits        45737    45728       -9     
- Misses       4383     4392       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.04% <ø> (ø)` | :arrow_up: |
| #single | `40.32% <ø> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=footer). Last update [15fa4bd...b527e9a](https://codecov.io/gh/pandas-dev/pandas/pull/17937?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

i think need to fix the NaT things first rather than halfway so this
you can pull out the isintance changes into a separate PR 
> i think need to fix the NaT things first rather than halfway so this
you can pull out the isintance changes into a separate PR

Are you saying you want the edits in core.indexes in a separate PR?  This is basically unrelated to #17793.
yes on both
OK, just reverted the changes in core.indexes, will put them in a separate PR.
Updated with one more method (`_validate_ops_compat`) moved out of the class and instead cdef'd as a function.
you might be able to move TImedelta completely to the new module as you now can import NaT directly w/o tslib and if you change the Timestamp checks to isinstance(obj, datetime).
> Most of the rest of _Timedelta&Timedelta can be moved into tslibs.timedeltas._Timedelta after #17793.

actually you already stated this, hahah


Yep.  I'd prefer to close this out and do the rest in a separate PR, but either way is OK.
I think let's move this all at once
OK.  The all-at-once needs to wait for #18014.

The reason I prefer to do this in two steps is because the follow up can be pure cut/paste, whereas this contains nonzero other changes that have already been reviewed.
(also this re-fixes a bunch of flake8 errors that got un-fixed in one of my rebase screwups)
give this a rebase. 
Suggestions?  This is on-deck to be a blocker
I'd like to move the entire Timedelta impl out of tslib.pyx
> I'd like to move the entire Timedelta impl out of tslib.pyx

So would I, but we're either going to have to settle for _almost_ all of it or for having an ugly circular-esque import with `Timestamp`.  If this gets merged I can get the rest of `_Timedelta` moved over later this evening.
The methods moved in this PR are cut/paste, whereas the other methods that need to be moved will take some tinkering, for a less trivial review process.  There is value in separating these steps.
ok, then pls add to your list moving the rest of Timedelta. It can be entirely defined in timedeltas.

I think its ok to import it *inside* a function if needed, you can probably do isinstance checks on timedelta/datetime as needed as well (rather than Timedelta). you can also maybe just duck-type check for Timestamp e.g.

```
def is_timestamp(obj):
   return isinstance(obj, datetime) and hasattr(obj, 'value')
```
> I think its ok to import it inside a function if needed

Great.  I had assumed that wouldn't be acceptable performance-wise, but never bothered to measure it.  That makes the remaining porting a lot simpler; pushing shortly.