Implement npy_dtime.pyx #17805

jbrockmendel · 2017-10-06T16:50:38Z

npy_dtime is a util-like module for tslibs. Actually, a better analogy would be src/datetime.pxd (which it ultimately is intended to replace).

Upcoming steps require _check_dts_bounds and OutOfBoundsDatetime be in a module upstream from tslib. The exception OutOfBoundsDatetime cannot be defined in a pxd file, so this can't just be shunted into src/datetime.pxd.

Using a .pyx file instead of a .pxd file is that dependency specification in setup.py gets less fragile. After updating e.g. tslibs.fields to cimport from npy_dtime, it will no longer need tseries_depends and pandas/_libs/src/datetime/np_datetime.c etc in its ext_data entry.

This also implements two small convenience functions dtstruct_to_dt64 and dt64_to_dtstruct that pass through to pandas_datetimestruct_to_datetime and pandas_datetime_to_datetimestruct respectively in the overwhelmingly-most-common case where unit=PANDAS_FR_ns. To keep the footprint small this PR only demonstrates their usage in one location in strptime. It's a lot less verbose, ultimately gets rid of a lot of unsightly wrapped lines.

pep8speaks · 2017-10-06T16:50:40Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on October 29, 2017 at 20:27 Hours UTC

codecov · 2017-10-07T02:12:31Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17805      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49978    49978              
==========================================
- Hits        45611    45602       -9     
- Misses       4367     4376       +9

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

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

codecov · 2017-10-07T02:12:33Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17805      +/-   ##
==========================================
- Coverage   91.24%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       50091    50099       +8     
==========================================
- Hits        45704    45703       -1     
- Misses       4387     4396       +9

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

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

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 5959ee3...e017a52. Read the comment docs.

jbrockmendel · 2017-10-07T18:48:45Z

The flake8 warning is pretty unavoidable here.

jbrockmendel · 2017-10-14T22:20:11Z

This is a blocker for upcoming work.

jreback · 2017-10-15T13:18:13Z

can we name this something else, maybe just spell it out

numpy_datetime or numpy_compat

jreback · 2017-10-15T13:19:08Z

np_datetime would be consistent

jbrockmendel · 2017-10-15T16:01:11Z

Changing to np_datetime, though with some misgivings about the exact name match.

jbrockmendel · 2017-10-21T03:34:35Z

suggestions for moving this down the field?

jreback · 2017-10-21T14:06:19Z

suggestions for moving this down the field?

what do you mean?

…libs-npy_dtime2

jbrockmendel · 2017-10-21T16:58:07Z

what do you mean?

The circleci errors are in test_grouped_box_multiple_axes and test_boxplot_legacy, which I think are unrelated, and the travis error is a flake8 complaint that is pretty unavoidable. Not sure what needs to be done to get this approved+merged.

jreback · 2017-10-21T19:39:06Z

The circleci errors are in test_grouped_box_multiple_axes and test_boxplot_legacy, which I think are unrelated, and the travis error is a flake8 complaint that is pretty unavoidable. Not sure what needs to be done to get this approved+merged.

did you rebase on master? these are working ok there.

jbrockmendel · 2017-10-21T20:05:54Z

did you rebase on master? these are working ok there.

Looks like that fixed the CircleCI error, good call. Looks like a build stalled on Travis; just took another swing at fixing the flake8 error and re-pushed. With a little luck we'll get to all-green.

jreback · 2017-10-21T21:10:22Z

fyi i always keep master up to date and anytime i push i rebase
makes it easier

jreback · 2017-10-21T21:11:37Z

pandas/_libs/tslib.pyx

@@ -58,6 +57,9 @@ from datetime cimport (
 from datetime import timedelta, datetime
 from datetime import time as datetime_time

+from tslibs.np_datetime cimport check_dts_bounds as _check_dts_bounds


is there a reason for the rename?

... because you told me to change it

the de-privatize is what I am referring. I don't recall asking for that and its not necessary.

Oh. De-privatized because it isn't actually private when is this module. But I'll revert it anyway.

de-privatize here (you did elsewhere)

jreback · 2017-10-21T21:12:45Z

pandas/_libs/tslibs/np_datetime.pxd

+# -*- coding: utf-8 -*-
+# cython: profile=False
+
+from numpy cimport int64_t, int32_t


you could actually just call this module util i think

I'd like to avoid name overlap with the existing libs/src/utilmodule. Also because in the dev branch I've ported src/util to a pure-cython (no C deps--> much simpler setup.py) tslibs.util. Don't want to get those mixed up.

My first choice is still the original npy_dtime, since np_datetime overlaps with the existing libs/src/datetime/np_datetime files.

jreback · 2017-10-23T23:28:45Z

pandas/_libs/tslibs/np_datetime.pyx

+    pass
+
+
+cdef inline _check_dts_bounds(pandas_datetimestruct *dts):


so this should be de-privatized

Happy to do it since that's how it was before: #17805 (comment)

…libs-npy_dtime2

jreback · 2017-10-28T15:35:35Z

pandas/_libs/tslib.pyx

@@ -58,6 +57,9 @@ from datetime cimport (
 from datetime import timedelta, datetime
 from datetime import time as datetime_time

+from tslibs.np_datetime cimport check_dts_bounds as _check_dts_bounds


de-privatize here (you did elsewhere)

jreback · 2017-10-28T15:35:57Z

pandas/_libs/tslibs/np_datetime.pxd

+        int32_t month, day, hour, min, sec, us, ps, as
+
+
+cdef check_dts_bounds(pandas_datetimestruct *dts)


you need a void here?

ok, saw your note, pls add to the list to add this as void (if possible / needed), not sure.

jreback · 2017-10-28T15:36:19Z

pandas/_libs/tslibs/np_datetime.pxd

+
+cdef check_dts_bounds(pandas_datetimestruct *dts)
+
+cdef int64_t dtstruct_to_dt64(pandas_datetimestruct* dts) nogil


where these nogil before?

The analogous src/datetime functions are, yes.

jreback · 2017-10-28T15:36:43Z

pandas/_libs/tslibs/np_datetime.pyx

+    pass
+
+
+cdef inline check_dts_bounds(pandas_datetimestruct *dts):


jreback · 2017-10-28T15:38:04Z

pandas/_libs/tslibs/strptime.pyx

+        iresult[i] = dtstruct_to_dt64(&dts)
+        try:
+            check_dts_bounds(&dts)
+        except ValueError:
            if is_coerce:
                iresult[i] = NPY_NAT
                continue
            else:


can remove the else (as we have a continue)

jreback · 2017-10-28T15:38:53Z

setup.py

@@ -493,6 +494,9 @@ def pxd(name):
                    'pxdfiles': ['_libs/src/util', '_libs/lib'],
                    'depends': tseries_depends,
                    'sources': npdt_srces},
+    '_libs.tslibs.np_datetime': {'pyxfile': '_libs/tslibs/np_datetime',
+                               'depends': tseries_depends[:2],


I would rather be explict here rather than depend on the ordering (you can also make a ctseries_depends or whatever and use that too).

jreback · 2017-10-28T15:39:24Z

setup.py

@@ -493,6 +494,9 @@ def pxd(name):
                    'pxdfiles': ['_libs/src/util', '_libs/lib'],
                    'depends': tseries_depends,
                    'sources': npdt_srces},
+    '_libs.tslibs.np_datetime': {'pyxfile': '_libs/tslibs/np_datetime',
+                               'depends': tseries_depends[:2],
+                               'sources': npdt_srces},


just spell things out, np_datetime_sources is much more readable (and change all references)

jreback · 2017-10-28T17:42:58Z

pandas/_libs/tslibs/np_datetime.pyx

+
+
+cdef inline void check_dts_bounds(pandas_datetimestruct *dts):
+    """Returns True if an error needs to be raised"""


can you add a doc-string, and change this (which is wrong), it will just raise if there is an oob date

jreback · 2017-10-28T17:44:14Z

pandas/_libs/tslibs/np_datetime.pyx

+                                               dts.day, dts.hour,
+                                               dts.min, dts.sec)
+        raise OutOfBoundsDatetime(
+            'Out of bounds nanosecond timestamp: %s' % fmt)


can you use {} in formatting. I would also pass dts to the OutOfBounds Constructor and format it there (a bit more idiomatic I think).

For now I'll implement that half of this that I know how to do; will work on my .format-fu and follow-up with the rest.

jbrockmendel · 2017-10-28T18:51:04Z

Adding the "void" to check_dts_bounds caused a bunch of errors because exceptions were ignored (i.e. no except -1). The current implementation in tslib doesn't have the void, so I just reverted that.

…libs-npy_dtime2

jreback · 2017-10-29T20:09:15Z

some linting errors.

jreback · 2017-10-29T21:36:57Z

thanks!

implement npy_dtime.pyx

53ec4d1

jbrockmendel added 2 commits October 6, 2017 10:31

whitespace fixup

67b3ec3

fixup typo

76cad9a

sinhrks added the Clean label Oct 6, 2017

flake8 fixup

ce7197d

jbrockmendel mentioned this pull request Oct 14, 2017

Separate conversion functions out from tslib #17875

Merged

Rename npy_dtime-->np_datetime

a1f8fc1

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

e4ce186

…libs-npy_dtime2

flake8 fixup

a345836

jreback reviewed Oct 21, 2017

View reviewed changes

jbrockmendel added 2 commits October 23, 2017 07:59

re-privatize _check_dts_bounds

c0b399e

dummy commit to force ci re-run

1791389

jreback reviewed Oct 23, 2017

View reviewed changes

re-de-privatize

16603d4

jorisvandenbossche added this to the 0.22.0 milestone Oct 24, 2017

jbrockmendel mentioned this pull request Oct 27, 2017

tslibs TODO list #17652

Closed

59 tasks

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

09a6f19

…libs-npy_dtime2

jreback requested changes Oct 28, 2017

View reviewed changes

jreback added the Datetime Datetime data dtype label Oct 28, 2017

edits per reviewer suggestions

c05cec7

jreback reviewed Oct 28, 2017

View reviewed changes

revert void; format per reviewer suggestion

c0b8216

jbrockmendel added 3 commits October 28, 2017 13:27

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

a9ca0b4

…libs-npy_dtime2

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

c3a1ce9

…libs-npy_dtime2

flake8 typo fixup

7ac9615

whitespace fixup

e017a52

jreback approved these changes Oct 29, 2017

View reviewed changes

jreback merged commit a355ed2 into pandas-dev:master Oct 29, 2017

peterpanmj pushed a commit to peterpanmj/pandas that referenced this pull request Oct 31, 2017

Implement npy_dtime.pyx (pandas-dev#17805)

311ad94

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

Implement npy_dtime.pyx (pandas-dev#17805)

bceeeeb

jbrockmendel deleted the tslibs-npy_dtime2 branch December 8, 2017 19:39

		pass


		cdef inline _check_dts_bounds(pandas_datetimestruct *dts):

		int32_t month, day, hour, min, sec, us, ps, as


		cdef check_dts_bounds(pandas_datetimestruct *dts)


		cdef check_dts_bounds(pandas_datetimestruct *dts)

		cdef int64_t dtstruct_to_dt64(pandas_datetimestruct* dts) nogil

		pass


		cdef inline check_dts_bounds(pandas_datetimestruct *dts):



		cdef inline void check_dts_bounds(pandas_datetimestruct *dts):
		"""Returns True if an error needs to be raised"""

Implement npy_dtime.pyx #17805

Implement npy_dtime.pyx #17805

Conversation

jbrockmendel commented Oct 6, 2017

pep8speaks commented Oct 6, 2017 • edited Loading

Comment last updated on October 29, 2017 at 20:27 Hours UTC

codecov bot commented Oct 7, 2017

Codecov Report

codecov bot commented Oct 7, 2017 • edited Loading

Codecov Report

jbrockmendel commented Oct 7, 2017

jbrockmendel commented Oct 14, 2017

jreback commented Oct 15, 2017

jreback commented Oct 15, 2017

jbrockmendel commented Oct 15, 2017

jbrockmendel commented Oct 21, 2017

jreback commented Oct 21, 2017

jbrockmendel commented Oct 21, 2017

jreback commented Oct 21, 2017

jbrockmendel commented Oct 21, 2017

jreback commented Oct 21, 2017

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

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

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

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

Choose a reason for hiding this comment

jbrockmendel commented Oct 28, 2017

jreback commented Oct 29, 2017

jreback commented Oct 29, 2017

pep8speaks commented Oct 6, 2017 •

edited

Loading

codecov bot commented Oct 7, 2017 •

edited

Loading