Handle "today" and "now" in cython instead of C #18666
Conversation
| return -1; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
This is an 49 line version of time.time()
| *out_local = 0; | ||
| } | ||
|
|
||
| return convert_datetime_to_datetimestruct(PANDAS_FR_s, rawtime, out); |
There was a problem hiding this comment.
Side-benefit: this is the only usage of convert_datetime_to_datetimestruct outside of np_datetime.c, so after this we can get rid of the extra layer there.
|
looks fine. can you run some asv's to see if anything changes significantly. (we should also have ones for Timestamp('now') and pd.to_datetime(['now']), if we don't already) |
Codecov Report
@@ Coverage Diff @@
## master #18666 +/- ##
=========================================
Coverage ? 91.58%
=========================================
Files ? 153
Lines ? 51272
Branches ? 0
=========================================
Hits ? 46960
Misses ? 4312
Partials ? 0
Continue to review full report at Codecov.
|
|
Yesterday morning the asvs were mixed leaning towards slightly faster post-PR. Now they're distinctly slower post-PR. Need to look into this. |
|
asv results are back to All Over The Place. ipython %timeit results make sense in py2 and are confusing in py3. In py2 %timeit shows a small slowdown in In py3 %timeit shows significant slowdowns across the board, including in Timestmap. No idea what the deal is there. |
|
Just pushed with an update that delays checking for "now" and "today" until as late as possible -- the idea being that these are fairly rare cases. Ran %timeit overnight to aggregate 10k runs for each of the relevant cases: Results: i.e. in all cases except Finally asv results: As mentioned elsewhere, |
| @@ -706,6 +716,19 @@ cpdef array_to_datetime(ndarray[object] values, errors='raise', | |||
| return oresult | |||
|
|
|||
|
|
|||
| cdef inline bint _parse_today_now(str val, int64_t* iresult): | |||
There was a problem hiding this comment.
we are moving these routines to conversion, yes? (obviously not in this PR)
|
thansk! |
|
Shoot, I was wrong, this does change the behavior of 0.21.0 Post-PR: I actually think this new behavior is better, but the intent of the PR was to be behavior-preserving. I can make a follow-up with a quick fix. LMK. |
|
ok great and add some explicit tests which lock things down (until / unless we change them) |
|
As in revert to previous behavior? |
|
revert to previous and put in place some tests for this. |
ATM the
Timestampconstructor (specificallytslibs.conversion.convert_str_to_tsobject) catches "now" and "today" before passing strings to_string_to_dts, which eventually dispatches tonp_datetime_strings. By contrast,to_datetime(specificallytslib.array_to_datetime) does not handle "now" or "today" internally.This PR makes
array_to_datetimehandle "now" and "today" internally, removing a big chunk of np_datetime_strings in the process.NB: This does not change the fact that
to_datetime("today") != Timestamp("today"), just makes this fact more obvious. My hope is that making this obvious will make it easier to change/fix in the future.asv results are as usual all over the place, but if anything look slightly faster.