Implement numpy_helper functions directly in cython #18059

jbrockmendel · 2017-11-01T02:50:37Z

cleanup unused bits of src/datetime.pxd

TL;DR this PR implements get_datetime64_value, get_timedelta64_value, and get_datetime64_unit directly in cython instead of getting them via C. There is no performance penalty. Weaning off of the src directory will make the build less of a hassle.

AFAICT the main reason why get_datetime64_unit, get_datetime64_value, and get_timedelta64_value (as well as is_float_object, is_datetime64_object`...) are implemented in numpy_helper.h and np_datetime.c is because cython/numpy does not make it easy to cimport the relevant symbols: https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/src/numpy.pxd#L481

    # Cannot be supported due to ## ## in macro:
    # bint PyArray_IsScalar(object, verbatim work)

After some wrangling and stackoverflowing, I found a way to implement these directly in cython. Importantly, the C code generated is functionally identical to the existing versions, so there shouldn't be a performance hit.

A big part of the diff comes from moving some declarations from np_datetime's pyx file to the pxd file.

cleanup unused bits of src/datetime.pxd

codecov · 2017-11-01T05:05:35Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #18059      +/-   ##
==========================================
- Coverage   91.25%   91.23%   -0.02%     
==========================================
  Files         163      163              
  Lines       50115    50115              
==========================================
- Hits        45730    45721       -9     
- Misses       4385     4394       +9

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

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <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 881ee30...ea4bef0. Read the comment docs.

codecov · 2017-11-01T05:05:40Z

Codecov Report

Merging #18059 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18059      +/-   ##
==========================================
+ Coverage   91.25%   91.26%   +<.01%     
==========================================
  Files         163      163              
  Lines       50124    50124              
==========================================
+ Hits        45742    45745       +3     
+ Misses       4382     4379       -3

Flag	Coverage Δ
#multiple	`89.07% <ø> (+0.02%)`	⬆️
#single	`40.32% <ø> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.1%)`	⬇️
pandas/plotting/_converter.py	`65.2% <0%> (+1.81%)`	⬆️

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 bc69dc6...13b85d8. Read the comment docs.

gfyoung · 2017-11-01T08:17:16Z

so there shouldn't be a performance hit.

Seem reasonable, but let's back that claim up with asv metrics to be sure.

jreback · 2017-11-01T10:36:36Z

instead of this, is there a reason you simply don't move datetime.pxd into np_datetime.pxd? (certainly can do some of the code cleanup, but after)

jbrockmendel · 2017-11-01T15:45:28Z

Seem reasonable, but let's back that claim up with asv metrics to be sure.

Will do. Even with affinity my asv results probably merit outside confirmation.

jbrockmendel · 2017-11-01T16:02:42Z

instead of this, is there a reason you simply don't move datetime.pxd into np_datetime.pxd? (certainly can do some of the code cleanup, but after)

That may be worth doing, but I see it as orthogonal to this. Cutting numpy_helper out of the equation simplifies things quite a bit (among other things I'm thinking about setup.py where 'include' and 'pxd' specifications seem pretty fragile)

jbrockmendel · 2017-11-01T16:45:16Z

With appropriate skepticism:

taskset 5 asv continuous -E virtualenv -f 1.1 master HEAD -b timestamp
[...]
      before           after         ratio
     [d80a4b81]       [ea4bef00]
+     5.95±0.02μs      6.79±0.05μs     1.14  timestamp.TimestampProperties.time_is_leap_year
+     5.71±0.03μs         6.28±1μs     1.10  timestamp.TimestampProperties.time_is_quarter_end
-     7.09±0.02μs      6.17±0.02μs     0.87  timestamp.TimestampProperties.time_is_year_start
-        195±50ns        109±0.2ns     0.56  timestamp.TimestampProperties.time_microsecond
-        862±40ms         159±10ms     0.18  binary_ops.Timeseries.time_timestamp_ops_diff2
-        443±10ms         77.5±4ms     0.18  binary_ops.TimeseriesTZ.time_timestamp_ops_diff2
-        469±30ms         63.1±7ms     0.13  binary_ops.TimeseriesTZ.time_timestamp_ops_diff1
-         155±8ms       19.1±0.2ms     0.12  binary_ops.Timeseries.time_timestamp_ops_diff1

taskset 5 asv continuous -E virtualenv -f 1.1 master HEAD -b timeseries
[...]
       before           after         ratio
     [d80a4b81]       [ea4bef00]
+      17.3±0.9ms         21.6±1ms     1.25  timeseries.TimeSeries.time_add_irregular
+     46.3±0.09μs       54.3±0.2μs     1.17  timeseries.SemiMonthOffset.time_begin_incr
+        132±10ms            150ms     1.13  timeseries.DatetimeIndex.time_dti_tz_factorize
-      20.3±0.1μs      18.4±0.09μs     0.90  timeseries.Offsets.time_custom_bday_incr
-      20.6±0.1μs       17.8±0.1μs     0.86  timeseries.Offsets.time_timeseries_day_apply

Digging out the C code produced by cython for the implementation of get_datetime64_value here (and stripping out the traceback comments)

static CYTHON_INLINE npy_datetime __pyx_f_6pandas_5_libs_6tslibs_11np_datetime_get_datetime64_value(PyObject *__pyx_v_obj) {
  npy_datetime __pyx_r;

  __pyx_r = ((PyDatetimeScalarObject *)__pyx_v_obj)->obval;
  goto __pyx_L0;

  /* function exit code */
  __pyx_L0:;
  return __pyx_r;
}

Whereas the version being replaced is:

PANDAS_INLINE npy_datetime get_datetime64_value(PyObject* obj) {
        return ((PyDatetimeScalarObject*)obj)->obval;
    }

cython is really verbose, but I pinged their mailing list and was told that the verbosity should be stripped out by the compiler.

Same story for get_datetime64_unit and get_timedelta64_value.

…libs-npy_dtime5

jreback · 2017-11-02T11:30:43Z

@chris-b1 can you review

chris-b1

Actual changes look fine to me

chris-b1 · 2017-11-02T23:17:23Z

pandas/_libs/tslibs/np_datetime.pxd

+    #    int64_t year
+    #    int32_t month, day, hour, min, sec, us, ps, as
+
+    ctypedef enum NPY_DATETIMEUNIT:


Is this enum needed?

No, it's leftover from an earlier revision. Will remove.

chris-b1 · 2017-11-02T23:19:39Z

pandas/_libs/tslibs/np_datetime.pyx

+# numpy object inspection
+
+cdef inline npy_datetime get_datetime64_value(object obj) nogil:
+    """


Can you make the docstrings more descriptive? Something like "returns int64 value underlying numpy datetime scalar Python object"

Good idea, will change.

jreback · 2017-11-03T00:04:18Z

rebase as always on re-push

…libs-npy_dtime5 update docstring per reviewer suggestion

…libs-npy_dtime5

jbrockmendel · 2017-11-06T00:24:10Z

Thoughts on this? I'd like to follow-up by moving a few more pieces of src/datetime.pxd over to np_datetime.pxd

gfyoung · 2017-11-06T00:54:02Z

ping @jreback

jreback · 2017-11-06T13:45:37Z

pandas/_libs/lib.pyx


+from tslibs.np_datetime cimport get_timedelta64_value, get_datetime64_value


so this now makes lib depend on np_datetime.pxd, which is ok as tseries_depends now lists this.

jreback · 2017-11-06T13:46:11Z

thanks! you will prob need to rebase your other PR's on this just to make sure.

jbrockmendel added 2 commits October 31, 2017 19:38

Implement numpy_helper functions directly in cython

c8f7158

cleanup unused bits of src/datetime.pxd

whitespace fixup

ea4bef0

gfyoung added Clean Internals Related to non-user accessible pandas implementation labels Nov 1, 2017

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

0b87865

…libs-npy_dtime5

chris-b1 approved these changes Nov 2, 2017

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

387bb44

…libs-npy_dtime5 update docstring per reviewer suggestion

This was referenced Nov 3, 2017

BLD: tslib dependencies not fully specified #18089

Closed

move implementation of Timedelta to tslibs.timedeltas #18085

Merged

Move normalization funcs up to conversion #18086

Merged

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

13b85d8

…libs-npy_dtime5

jreback reviewed Nov 6, 2017

View reviewed changes

jreback added this to the 0.22.0 milestone Nov 6, 2017

jreback merged commit f17aa26 into pandas-dev:master Nov 6, 2017

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

Implement numpy_helper functions directly in cython (pandas-dev#18059)

fba54c2

jbrockmendel deleted the tslibs-npy_dtime5 branch December 8, 2017 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement numpy_helper functions directly in cython #18059

Implement numpy_helper functions directly in cython #18059

jbrockmendel commented Nov 1, 2017

codecov bot commented Nov 1, 2017

codecov bot commented Nov 1, 2017 •

edited

Loading

gfyoung commented Nov 1, 2017 •

edited

Loading

jreback commented Nov 1, 2017

jbrockmendel commented Nov 1, 2017

jbrockmendel commented Nov 1, 2017

jbrockmendel commented Nov 1, 2017

jreback commented Nov 2, 2017

chris-b1 left a comment

chris-b1 Nov 2, 2017

jbrockmendel Nov 2, 2017

chris-b1 Nov 2, 2017

jbrockmendel Nov 3, 2017

jreback commented Nov 3, 2017

jbrockmendel commented Nov 6, 2017

gfyoung commented Nov 6, 2017

jreback Nov 6, 2017

jreback commented Nov 6, 2017


		from tslibs.np_datetime cimport get_timedelta64_value, get_datetime64_value

Implement numpy_helper functions directly in cython #18059

Implement numpy_helper functions directly in cython #18059

Conversation

jbrockmendel commented Nov 1, 2017

codecov bot commented Nov 1, 2017

Codecov Report

codecov bot commented Nov 1, 2017 • edited Loading

Codecov Report

gfyoung commented Nov 1, 2017 • edited Loading

jreback commented Nov 1, 2017

jbrockmendel commented Nov 1, 2017

jbrockmendel commented Nov 1, 2017

jbrockmendel commented Nov 1, 2017

jreback commented Nov 2, 2017

chris-b1 left a comment

Choose a reason for hiding this comment

chris-b1 Nov 2, 2017

Choose a reason for hiding this comment

jbrockmendel Nov 2, 2017

Choose a reason for hiding this comment

chris-b1 Nov 2, 2017

Choose a reason for hiding this comment

jbrockmendel Nov 3, 2017

Choose a reason for hiding this comment

jreback commented Nov 3, 2017

jbrockmendel commented Nov 6, 2017

gfyoung commented Nov 6, 2017

jreback Nov 6, 2017

Choose a reason for hiding this comment

jreback commented Nov 6, 2017

codecov bot commented Nov 1, 2017 •

edited

Loading

gfyoung commented Nov 1, 2017 •

edited

Loading