CLN/PERF: simplify tslib.get_time_micros

[jbrockmendel]

tslib.get_time_micros implementation is way more complicated than it needs to be. Instead of calling dt64_to_dtstruct in a loop, this changes the function to one call to np.mod.

AFAIK the np.mod call is to the python function, not sure if numpy exposes a cython version of mod. Then again, the function is only used once in indexes.datetimes and isn't all that perf sensitive. It might not need to be in cython in the first place.

Re-wrote inaccurate docstring.

[gfyoung]

asv benchmarks as usual before we can merge this.

[jbrockmendel]

asv benchmarks as usual before we can merge this.

Since its just one function, how about just a timeit result

0.21.0

In [1]: import numpy as np
In [2]: import pandas as pd
In [3]: np.random.seed(5678)
In [4]: arr = (np.random.randn(10000) * 1e7).astype(np.int64)
In [5]: %timeit res = tslib.get_time_micros(arr)
1000 loops, best of 3: 322 µs per loop

PR:

In [5]: %timeit res = tslib.get_time_micros(arr)
1000 loops, best of 3: 238 µs per loop

[codecov]

Codecov Report

Merging #18389 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18389      +/-   ##
==========================================
- Coverage   91.35%   91.34%   -0.02%     
==========================================
  Files         163      163              
  Lines       49695    49695              
==========================================
- Hits        45401    45392       -9     
- Misses       4294     4303       +9

Flag	Coverage Δ
#multiple	89.14% <100%> (ø)	⬆️
#single	39.66% <0%> (-0.07%)	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	95.49% <100%> (ø)	⬆️
pandas/io/gbq.py	25% <0%> (-58.34%)	⬇️
pandas/core/frame.py	97.8% <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 e6eac0b...26ec1f3. Read the comment docs.

[jreback]

pretty sure we have a constant somewhere rather than hard-coding 86400.....

also shouldn't this routine be in conversion?

[jbrockmendel]

conversion would be reasonable. fields might be a better fit. I'm actually leaning towards "this doesn't need to be a function" since its a one-linear and only used once.

The constant you're thinking of is defined in conversion.

[jreback]

needs an update according to comments

[jbrockmendel]

No thoughts on the "just put this one-liner in the one place its used" idea?

[jreback]

don't we have this constant defined already somewhere?

[jbrockmendel]

Yah, it’s defined in conversion. But it’s all the same to the compiler right? If it was something that was liable to change and needed to be kept in sync (eg nat_strings) it would make sense to cimport it. But for a constant like this it isn’t worth the hassle; it’d be like defining DAYSPERWEEK=7 and importing that all over the place.

[jreback]

No thoughts on the "just put this one-liner in the one place its used" idea?

yeah, but rather actually have these defined in a single place (rather than scattered about), so in same vein could move _to_time_micros here as well.

[jbrockmendel]

yeah, but rather actually have these defined in a single place (rather than scattered about),

I'm with you on the non-scattering. Anyway, moved this to fields a few minutes ago.

[jreback]

ok, let's add to list to remove need for this