BUG: Fix Timestamp type checks to work with subclassed datetime (#25851)
State: closed

- [x] closes #25851
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

PyDateTime_CheckExact is being used as a proxy for isinstance(obj, Timestamp) checks for performance reasons. However, if a subclass of the std datetime lib is being used, these checks are not sufficient to determine if an object is a Timestamp or not.

As discussed in #25746, any solution will be less performant than PyDateTime_CheckExact.  The best solution found was to use isinstance(obj, _Timestamp), and that is what I implemented here.

Of course, a few additional changes were necessary to be able to use that check.

Since asv isn't very good for cython code, I've done performance testing on all the modified functions by running them 10,000 times in a function, then timing that function in `%timeit`.  The results are below:
```
                       ----------- PR ---------         -------- Master -------
                        datetime     timestamp           datetime    timestamp
array_to_datetime        34.3 ms       34.5 ms            34.2 ms      34.5 ms
localize_pydatetime        15 ms       67.7 ms            15.2 ms      57.5 ms
normalize_date           12.7 ms         91 ms            12.8 ms      78.3 ms
Timedelta addition       1.05 ms         23 ms            1.04 ms      22.9 ms
Timestamp constructor    11.1 ms       9.85 ms            10.9 ms      9.79 ms
tz_localize                  N/A       63.7 ms                N/A      53.5 ms
normalize                    N/A       90.6 ms                N/A      78.5 ms
```
As you can see, some of the functions used with a `Timestamp` are affected, with a few unaffected.  Where there is an impact, it appears to be 15-20% less performant.

Note this fix also fixes (potentially) #25734, but if we want a separate test case for that issue I could modify my existing PR for that.

Labels:
name: Datetime
description: Datetime data dtype
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
Hello @ArtificialQualia! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

















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

##### Comment last updated at 2019-04-05 15:33:46 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=h1) Report
> Merging [#25853](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6e0f9a971bcd07a4a6283dc03190b4363d42e292?src=pr&el=desc) will **decrease** coverage by `49.63%`.
> The diff coverage is `100%`.

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

```diff
@@             Coverage Diff             @@
##           master   #25853       +/-   ##
===========================================
- Coverage   91.45%   41.81%   -49.64%     
===========================================
  Files         172      172               
  Lines       52892    52892               
===========================================
- Hits        48373    22119    -26254     
- Misses       4519    30773    +26254
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.81% <100%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/\_libs/tslibs/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL19saWJzL3RzbGlicy9fX2luaXRfXy5weQ==) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.36%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| [pandas/core/tools/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvbnVtZXJpYy5weQ==) | `10.44% <0%> (-89.56%)` | :arrow_down: |
| ... and [130 more](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25853?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/25853?src=pr&el=footer). Last update [6e0f9a9...4447746](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=h1) Report
> Merging [#25853](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0b031306746511130f19e6af33ca317f942f862c?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #25853      +/-   ##
==========================================
- Coverage   40.85%   40.72%   -0.13%     
==========================================
  Files         175      175              
  Lines       52552    52551       -1     
==========================================
- Hits        21468    21401      -67     
- Misses      31084    31150      +66
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `40.72% <100%> (-0.13%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `42.73% <100%> (+0.1%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-50%)` | :arrow_down: |
| [pandas/io/formats/format.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvZm9ybWF0LnB5) | `30.44% <0%> (-4.14%)` | :arrow_down: |
| [pandas/io/formats/csvs.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvY3N2cy5weQ==) | `67.06% <0%> (-1.2%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `51.2% <0%> (-1.11%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `48.26% <0%> (-1.06%)` | :arrow_down: |
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `49% <0%> (-0.34%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `45.66% <0%> (-0.21%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `34.51% <0%> (-0.18%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `65.93% <0%> (-0.16%)` | :arrow_down: |
| ... and [3 more](https://codecov.io/gh/pandas-dev/pandas/pull/25853/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25853?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/25853?src=pr&el=footer). Last update [0b03130...ded5e69](https://codecov.io/gh/pandas-dev/pandas/pull/25853?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

below comments identify areas where _Timestamp has changed to prevent having to have circular imports.
@jbrockmendel separated out _Timestamp as you suggested.  Obviously had to make a number of other changes to support that, let me know if they are acceptable, or what other changes you'd like.

Didn't mean to make most of those comments 'reviews', but they do need to be reviewed anyways I suppose!
Sorry, I realize I did most of my comments as a 'review' again in this so they aren't showing up.  I'll re-add them again.
@ArtificialQualia  can you merge master and rename _timestamp.pyx -> c_timestamp.pyx, otherwise lgtm.
Master merged and file renamed.

Build seems to be failing because of google's cloud API?  Not sure what that is about.

I think this merge might want to wait on #25938 so that function can be imported and used rather than duplicating logic in `_Timestamp`.
@ArtificialQualia yeah don't worry about the gbq failure.

ok will have you rebase after # 25938
@ArtificialQualia if you'd merge master; ping on green.
@jreback merged master and green
lgtm. ping on green!
@jreback green
thanks @ArtificialQualia nice patch! keep em coming!
Still a -1 to backport this to 0.24.x @jreback? My coworkers could use this patch to upgrade to 0.24 as the regression was introduced recently.