Implement npy_dtime.pyx #17805
Implement npy_dtime.pyx #17805
Conversation
|
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 Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
The flake8 warning is pretty unavoidable here. |
|
This is a blocker for upcoming work. |
|
can we name this something else, maybe just spell it out numpy_datetime or numpy_compat |
|
np_datetime would be consistent |
|
Changing to np_datetime, though with some misgivings about the exact name match. |
|
suggestions for moving this down the field? |
what do you mean? |
…libs-npy_dtime2
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. |
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. |
|
fyi i always keep master up to date and anytime i push i rebase |
pandas/_libs/tslib.pyx
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
is there a reason for the rename?
There was a problem hiding this comment.
... because you told me to change it
There was a problem hiding this comment.
the de-privatize is what I am referring. I don't recall asking for that and its not necessary.
There was a problem hiding this comment.
Oh. De-privatized because it isn't actually private when is this module. But I'll revert it anyway.
There was a problem hiding this comment.
de-privatize here (you did elsewhere)
| # -*- coding: utf-8 -*- | ||
| # cython: profile=False | ||
|
|
||
| from numpy cimport int64_t, int32_t |
There was a problem hiding this comment.
you could actually just call this module util i think
There was a problem hiding this comment.
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.
pandas/_libs/tslibs/np_datetime.pyx
Outdated
| pass | ||
|
|
||
|
|
||
| cdef inline _check_dts_bounds(pandas_datetimestruct *dts): |
There was a problem hiding this comment.
so this should be de-privatized
There was a problem hiding this comment.
Happy to do it since that's how it was before: #17805 (comment)
…libs-npy_dtime2
pandas/_libs/tslib.pyx
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
de-privatize here (you did elsewhere)
| int32_t month, day, hour, min, sec, us, ps, as | ||
|
|
||
|
|
||
| cdef check_dts_bounds(pandas_datetimestruct *dts) |
There was a problem hiding this comment.
ok, saw your note, pls add to the list to add this as void (if possible / needed), not sure.
|
|
||
| cdef check_dts_bounds(pandas_datetimestruct *dts) | ||
|
|
||
| cdef int64_t dtstruct_to_dt64(pandas_datetimestruct* dts) nogil |
There was a problem hiding this comment.
The analogous src/datetime functions are, yes.
| pass | ||
|
|
||
|
|
||
| cdef inline check_dts_bounds(pandas_datetimestruct *dts): |
pandas/_libs/tslibs/strptime.pyx
Outdated
| iresult[i] = dtstruct_to_dt64(&dts) | ||
| try: | ||
| check_dts_bounds(&dts) | ||
| except ValueError: | ||
| if is_coerce: | ||
| iresult[i] = NPY_NAT | ||
| continue | ||
| else: |
There was a problem hiding this comment.
can remove the else (as we have a continue)
setup.py
Outdated
| @@ -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], | |||
There was a problem hiding this comment.
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).
setup.py
Outdated
| @@ -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}, | |||
There was a problem hiding this comment.
just spell things out, np_datetime_sources is much more readable (and change all references)
pandas/_libs/tslibs/np_datetime.pyx
Outdated
|
|
||
|
|
||
| cdef inline void check_dts_bounds(pandas_datetimestruct *dts): | ||
| """Returns True if an error needs to be raised""" |
There was a problem hiding this comment.
can you add a doc-string, and change this (which is wrong), it will just raise if there is an oob date
pandas/_libs/tslibs/np_datetime.pyx
Outdated
| dts.day, dts.hour, | ||
| dts.min, dts.sec) | ||
| raise OutOfBoundsDatetime( | ||
| 'Out of bounds nanosecond timestamp: %s' % fmt) |
There was a problem hiding this comment.
can you use {} in formatting. I would also pass dts to the OutOfBounds Constructor and format it there (a bit more idiomatic I think).
There was a problem hiding this comment.
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.
|
Adding the "void" to |
|
some linting errors. |
|
thanks! |
npy_dtimeis autil-like module fortslibs. Actually, a better analogy would be src/datetime.pxd (which it ultimately is intended to replace).Upcoming steps require
_check_dts_boundsandOutOfBoundsDatetimebe in a module upstream fromtslib. The exceptionOutOfBoundsDatetimecannot 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.fieldstocimportfromnpy_dtime, it will no longer needtseries_dependsandpandas/_libs/src/datetime/np_datetime.cetc in itsext_dataentry.This also implements two small convenience functions
dtstruct_to_dt64anddt64_to_dtstructthat pass through topandas_datetimestruct_to_datetimeandpandas_datetime_to_datetimestructrespectively in the overwhelmingly-most-common case whereunit=PANDAS_FR_ns. To keep the footprint small this PR only demonstrates their usage in one location instrptime. It's a lot less verbose, ultimately gets rid of a lot of unsightly wrapped lines.