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

ArtificialQualia · 2019-03-23T23:06:55Z

closes Fix PyDateTime_CheckExact checks as proxy for Timestamp checks in cython files #25851
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
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.

…as-dev#25851)

pep8speaks · 2019-03-23T23:06:59Z

Hello @ArtificialQualia! 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 2019-04-05 15:33:46 UTC

pandas/_libs/tslibs/__init__.py

codecov · 2019-03-23T23:32:36Z

Codecov Report

Merging #25853 into master will decrease coverage by 49.63%.
The diff coverage is 100%.

@@             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%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/_libs/tslibs/__init__.py	`100% <100%> (ø)`	⬆️
pandas/io/formats/latex.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/plotting.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.36%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-95.46%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.17%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.15%)`	⬇️
pandas/core/tools/numeric.py	`10.44% <0%> (-89.56%)`	⬇️
... and 130 more

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 6e0f9a9...4447746. Read the comment docs.

codecov · 2019-03-23T23:32:37Z

Codecov Report

Merging #25853 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

@@            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%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimelike.py	`42.73% <100%> (+0.1%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-50%)`	⬇️
pandas/io/formats/format.py	`30.44% <0%> (-4.14%)`	⬇️
pandas/io/formats/csvs.py	`67.06% <0%> (-1.2%)`	⬇️
pandas/core/internals/blocks.py	`51.2% <0%> (-1.11%)`	⬇️
pandas/util/testing.py	`48.26% <0%> (-1.06%)`	⬇️
pandas/core/dtypes/cast.py	`49% <0%> (-0.34%)`	⬇️
pandas/core/series.py	`45.66% <0%> (-0.21%)`	⬇️
pandas/core/frame.py	`34.51% <0%> (-0.18%)`	⬇️
pandas/core/arrays/datetimes.py	`65.93% <0%> (-0.16%)`	⬇️
... and 3 more

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 0b03130...ded5e69. Read the comment docs.

pandas/_libs/tslibs/conversion.pyx

pandas/_libs/tslibs/timedeltas.pyx

pandas/_libs/tslibs/timestamps.pyx

pandas/tests/arithmetic/test_datetime64.py

ArtificialQualia · 2019-03-28T01:43:41Z

below comments identify areas where _Timestamp has changed to prevent having to have circular imports.

pandas/_libs/tslibs/_timestamp.pyx

ArtificialQualia · 2019-03-28T02:05:36Z

@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!

pandas/_libs/tslibs/timestamps.pyx

pandas/tests/tslibs/test_normalize_date.py

pandas/_libs/tslibs/_timestamp.pyx

jbrockmendel · 2019-03-29T15:36:20Z

pandas/_libs/tslibs/timestamps.pyx

+        Helper to create a Timedelta so that the
+        Timedelta class doesn't have to be imported elsewhere
+        """
+        return Timedelta(*args, **kwargs)


I'd rather just have the runtime import in TImedelta.resolution (I think the only place this is used). Or for that matter just move resolution from _Timestamp to Timestamp.

It's used in two places in _Timestamp, _Timestamp.__sub__ and _Timestamp.resolution. If we want to go down the runtime import path, we could either add the Timedelta import in both functions in _Timestamp, or add a _Timestamp import into timedeltas._binary_op_method_timedeltalike

As for moving the _Timestamp functions to Timestamp, _Timestamp.resolution can be moved with no issues. However, _Timestamp.__sub__ can not. Not sure where the problem is there, but if you want I can try to figure that one out.

right that's what I mean, it doens't need to be in pandas/_libs/tslibs/__init__.py, rather just import it directly where needed. its ok where its living.

…alQualia/pandas into update_timestamp_checks

ArtificialQualia · 2019-03-31T12:24:02Z

pandas/_libs/tslibs/_timestamp.pyx

+            elif PyDelta_Check(other):
+                nanos = (other.days * 24 * 60 * 60 * 1000000 +
+                         other.seconds * 1000000 +
+                         other.microseconds) * 1000


Ugh, this comment got dropped again due to 'review'. I've ensured this was the last missing comment I had.

As comment above implies, this logic was copied from delta_to_nanoseconds, has also been cleaned up of a few unnecessary ifs.

delta_to_nanoseconds is a very independent function though, and if we want to move that to a separate file that would work too.

jreback · 2019-04-05T00:48:05Z

@ArtificialQualia can you merge master and rename _timestamp.pyx -> c_timestamp.pyx, otherwise lgtm.

ArtificialQualia · 2019-04-05T02:06:21Z

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.

jreback · 2019-04-05T02:08:23Z

@ArtificialQualia yeah don't worry about the gbq failure.

ok will have you rebase after # 25938

jreback · 2019-04-05T02:28:55Z

@ArtificialQualia if you'd merge master; ping on green.

ArtificialQualia · 2019-04-05T12:17:46Z

@jreback merged master and green

jreback · 2019-04-05T13:13:01Z

pandas/_libs/tslibs/__init__.py

@@ -8,3 +8,4 @@
 from .timestamps import Timestamp
 from .timedeltas import delta_to_nanoseconds, ints_to_pytimedelta, Timedelta
 from .tzconversion import tz_convert_single
+from .c_timestamp import maybe_integer_op_deprecated # isort:skip


do we really need this function here? this should not be exposed to the rest of the codebase (or directly imported if it actually is needed)

maybe just expose it in timestamp.pyx itself to make it easier to import

It was exposed previously, and is only used in pandas/core/arrays/datetimelike.py. _Timestamp itself makes use of it, so it has to be in c_timestamp.pyx otherwise we would have a circular import.

I can change the import in datetimelike.py to directly import it if you want.

sorry made the comment in the wrong place. yes just import it directly in dateimelike.py and don't put it in tslibs/__init__.py

jreback · 2019-04-05T13:13:38Z

pandas/_libs/tslibs/c_timestamp.pxd

+        int64_t value, nanosecond
+        object freq
+        list _date_attributes
+    cpdef bint _get_start_end_field(self, str field)


future PR we should rename the 'private' functions to public if they are actually exposed (the _ leading ones)

jreback · 2019-04-05T15:46:10Z

lgtm. ping on green!

ArtificialQualia · 2019-04-05T16:24:16Z

@jreback green

jreback · 2019-04-05T16:54:53Z

thanks @ArtificialQualia nice patch! keep em coming!

mroeschke · 2019-04-05T18:10:34Z

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.

BUG: Fix Timestamp type checks to work with subclassed datetime (pand…

2e5a160

…as-dev#25851)

increase spacing in comment

82b2420

ArtificialQualia commented Mar 23, 2019

View reviewed changes

pandas/_libs/tslibs/__init__.py Outdated Show resolved Hide resolved

fix spacing

4447746

fix import formatting

2d20375

jbrockmendel reviewed Mar 24, 2019

View reviewed changes

pandas/_libs/tslibs/conversion.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 24, 2019

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 24, 2019

View reviewed changes

pandas/_libs/tslibs/timestamps.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 24, 2019

View reviewed changes

pandas/tests/arithmetic/test_datetime64.py Show resolved Hide resolved

gfyoung added Datetime Datetime data dtype Internals Related to non-user accessible pandas implementation labels Mar 25, 2019

move _Timestamp to separate file to prevent circular import

8e8d816

ArtificialQualia commented Mar 28, 2019

View reviewed changes

pandas/_libs/tslibs/_timestamp.pyx Outdated Show resolved Hide resolved

ArtificialQualia commented Mar 28, 2019

View reviewed changes

pandas/_libs/tslibs/_timestamp.pyx Outdated Show resolved Hide resolved

ArtificialQualia added 3 commits March 27, 2019 22:07

fix indenting

4fd35ac

fix line spacing

354d3a3

kickoff new build

600be4f

jreback requested changes Mar 29, 2019

View reviewed changes

pandas/_libs/tslibs/timestamps.pyx Show resolved Hide resolved

jbrockmendel reviewed Mar 29, 2019

View reviewed changes

pandas/tests/tslibs/test_normalize_date.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 29, 2019

View reviewed changes

pandas/_libs/tslibs/_timestamp.pyx Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 29, 2019

View reviewed changes

ArtificialQualia and others added 4 commits March 29, 2019 17:26

updates for PR review

7a60942

Merge branch 'master' into update_timestamp_checks

dd5d492

fix isort error

46f306d

Merge branch 'update_timestamp_checks' of https://github.com/Artifici…

7268a91

…alQualia/pandas into update_timestamp_checks

fix namespace test for new api

dc0f806

jbrockmendel mentioned this pull request Mar 31, 2019

REF: Separate timezone-conversion from tslibs.conversion #25938

Merged

fix typo

0cad612

ArtificialQualia commented Mar 31, 2019

View reviewed changes

ArtificialQualia and others added 3 commits April 4, 2019 21:17

Merge branch 'master' into update_timestamp_checks

a1cb913

merge changes into _timestamp

ee9d2d5

change _timestamp files to c_timestamp

560ef6f

jreback added this to the 0.25.0 milestone Apr 5, 2019

ArtificialQualia and others added 3 commits April 4, 2019 22:38

Merge branch 'master' into update_timestamp_checks

92a9c52

use new tzconversion.tz_convert_single merged function

b87aa99

move imports to fix import exceptions

11a091a

jreback requested changes Apr 5, 2019

View reviewed changes

ArtificialQualia added 4 commits April 5, 2019 10:18

change imports

4896568

fix api test

43ec600

force rebuild

9795807

force rebuild

ded5e69

jreback approved these changes Apr 5, 2019

View reviewed changes

jreback merged commit 181f972 into pandas-dev:master Apr 5, 2019

This was referenced Apr 5, 2019

BUG: Fix localize_pydatetime using meta datetimes as Timestamp (#25734) #25746

Closed

Pandas 0.24.x throws an error when using freezegun with CustomBusinessDay #25734

Closed

spulec mentioned this pull request Jul 9, 2019

Failure with new Pandas 0.24.x: 'FakeDatetime' object has no attribute 'tz_localize' spulec/freezegun#284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ArtificialQualia commented Mar 23, 2019 •

edited

Loading

pep8speaks commented Mar 23, 2019 •

edited

Loading

codecov bot commented Mar 23, 2019

codecov bot commented Mar 23, 2019 •

edited

Loading

ArtificialQualia commented Mar 28, 2019

ArtificialQualia commented Mar 28, 2019

jbrockmendel Mar 29, 2019

ArtificialQualia Mar 30, 2019

jreback Apr 5, 2019

ArtificialQualia Mar 31, 2019

jreback commented Apr 5, 2019 •

edited

Loading

ArtificialQualia commented Apr 5, 2019

jreback commented Apr 5, 2019

jreback commented Apr 5, 2019

ArtificialQualia commented Apr 5, 2019

jreback Apr 5, 2019

jreback Apr 5, 2019

ArtificialQualia Apr 5, 2019

jreback Apr 5, 2019

jreback Apr 5, 2019

jreback commented Apr 5, 2019

ArtificialQualia commented Apr 5, 2019

jreback commented Apr 5, 2019

mroeschke commented Apr 5, 2019 •

edited

Loading

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

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

Conversation

ArtificialQualia commented Mar 23, 2019 • edited Loading

pep8speaks commented Mar 23, 2019 • edited Loading

Comment last updated at 2019-04-05 15:33:46 UTC

codecov bot commented Mar 23, 2019

Codecov Report

codecov bot commented Mar 23, 2019 • edited Loading

Codecov Report

ArtificialQualia commented Mar 28, 2019

ArtificialQualia commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Apr 5, 2019 • edited Loading

ArtificialQualia commented Apr 5, 2019

jreback commented Apr 5, 2019

jreback commented Apr 5, 2019

ArtificialQualia commented Apr 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Apr 5, 2019

ArtificialQualia commented Apr 5, 2019

jreback commented Apr 5, 2019

mroeschke commented Apr 5, 2019 • edited Loading

ArtificialQualia commented Mar 23, 2019 •

edited

Loading

pep8speaks commented Mar 23, 2019 •

edited

Loading

codecov bot commented Mar 23, 2019 •

edited

Loading

jreback commented Apr 5, 2019 •

edited

Loading

mroeschke commented Apr 5, 2019 •

edited

Loading