have _NaT subclass datetime directly

[jbrockmendel]

The main change made by this PR is to have _NaT subclass datetime directly instead of subclassing _Timestamp. Everything else is just to preserve the current behavior.

Why you ask? Because once this is OKed, the entire NaT machinery can be cut/paste into a stand-alone module whose only intra-pandas dependency is util.

The only non-trivial behavior is in __add__ and __sub__, where currently _NaT dispatches to _Timestamp for a few cases. This PR re-implements those cases directly in _NaT. While that does mean a small amount of repetition, it opens up the possibility of using @cython.final for potential optimization in _Timestamp.__add__/__sub__.

NaTType.to_datetime could use _make_nat_func if we were OK with it not producing the same warning as Timestamp.to_datetime.

• closes NaT inheritance #17435
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[codecov]

Codecov Report

Merging #17793 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17793      +/-   ##
==========================================
- Coverage   91.24%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       49918    49918              
==========================================
- Hits        45546    45537       -9     
- Misses       4372     4381       +9

Flag	Coverage Δ
#multiple	89.02% <ø> (ø)	⬆️
#single	40.24% <ø> (-0.07%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	25% <0%> (-58.34%)	⬇️
pandas/core/frame.py	97.73% <0%> (-0.1%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37860a5...d299ab5. Read the comment docs.

[codecov]

Codecov Report

Merging #17793 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17793      +/-   ##
==========================================
- Coverage   91.24%   91.23%   -0.02%     
==========================================
  Files         163      163              
  Lines       50168    50168              
==========================================
- Hits        45777    45769       -8     
- Misses       4391     4399       +8

Flag	Coverage Δ
#multiple	89.04% <ø> (ø)	⬆️
#single	40.27% <ø> (-0.06%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	25% <0%> (-58.34%)	⬇️
pandas/core/frame.py	97.75% <0%> (-0.1%)	⬇️
pandas/core/indexes/datetimes.py	95.5% <0%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4489389...6dce07f. Read the comment docs.

[jreback]

going to wait on this till after we release 0.21.0

[jreback]

note that we might as well NOT subclass datetime at all! its a bit tricky though you would have to implement a metaclass so isinstance check would work (IOW you still want this to be a datetime). but that makes this simpler.

[jbrockmendel]

its a bit tricky though you would have to implement a metaclass so isinstance check would work

My impression is that metaclasses in cython are finicky. That might be out of date.

going to wait on this till after we release 0.21.0

Sounds good. In the interim I'll try to finish off the offsets and go down the TODO list.

[jreback]

i would like to see this not subclass datetime
then this becomes really simple; would still have to be able to masquerade as a datetime/timedelta
for isintsance checks

you can dig into the CPython source to see how this is done

[jbrockmendel]

i would like to see this not subclass datetime

I'll put looking into this on the tslibs todo list.

then this becomes really simple; would still have to be able to masquerade as a datetime/timedelta for isintsance checks

At the moment isinstance(NaT, Timedelta) returns False. Is it supposed to be True?

[jreback]

At the moment isinstance(NaT, Timedelta) returns False. Is it supposed to be True?

hmm maybe we don’t event need to make isinstance be true; so try not subclassing datetime and see what breaks;

[jbrockmendel]

hmm maybe we don’t event need to make isinstance be true; so try not subclassing datetime and see what breaks;

Do you mind saving that for a follow-up? This has been added to #17652.

[jreback]

ok this lgtm.

[jreback]

@jbrockmendel if you want to merge in different order that my arbitrary one :> lmk. just ping which one should go first.

[jbrockmendel]

I'll be happy with any order; go nuts. Way to be a closer today!

…

Sent from my iPhone

On Oct 27, 2017, at 5:01 PM, Jeff Reback ***@***.***> wrote: @jbrockmendel if you want to merge in different order that my arbitrary one :> lmk. just ping which one should go first. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jreback]

need’s rebase

[jreback]

thanks!