BUG/PERF: offsets.apply doesnt preserve nanosecond #7697

sinhrks · 2014-07-08T15:55:25Z

Main Fix is to preserve nanosecond info which can lost during offset.apply, but it also includes:

Support dateutil timezone
Little performance improvement. Even though v0.14.1 should take longer than v0.14.0 because perf test in v0.14 doesn't perform timestamp conversion which was fixed in BUG: offsets.apply may return datetime #7502.
NOTE: This caches Tick.delta because it was calculated 3 times repeatedly, but does it cause any side effect?

Before

-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
timeseries_year_incr                         |   0.0164 |   0.0103 |   1.5846 |
timeseries_year_apply                        |   0.0153 |   0.0094 |   1.6356 |
timeseries_day_incr                          |   0.0187 |   0.0053 |   3.5075 |
timeseries_day_apply                         |   0.0164 |   0.0033 |   4.9048 |

Target [d0076db] : PERF: Improve index.min and max perf
Base   [da0f7ae] : RLS: 0.14.0 final

After the fix

-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
timeseries_year_incr                         |   0.0150 |   0.0087 |   1.7339 |
timeseries_year_apply                        |   0.0126 |   0.0073 |   1.7283 |
timeseries_day_incr                          |   0.0130 |   0.0053 |   2.4478 |
timeseries_day_apply                         |   0.0107 |   0.0033 |   3.2143 |

Target [64dd021] : BUG: offsets.apply doesnt preserve nanosecond
Base   [da0f7ae] : RLS: 0.14.0 final

jreback · 2014-07-08T15:57:45Z

pandas/tseries/offsets.py

@@ -26,15 +27,17 @@

 # convert to/from datetime/timestamp to allow invalid Timestamp ranges to pass thru
 def as_timestamp(obj):
+    if isinstance(obj, Timestamp):
+        return obj
+    if type(obj) == date:


shouldn't this be isinstance(obj, date)? not sure if you need to include (np.datetime64,datetime,date)

Because `datetimeis a subclass ofdate``. Modified to use``isinstance``.

>>> dt = datetime.datetime(2011, 1, 1) >>> isinstance(dt, datetime.date) True

Ah, Timestamp can accept datetime.date. Thus this check seems not to be required at all.

jreback · 2014-07-21T11:50:29Z

@sinhrks revisit?

sinhrks · 2014-07-21T15:06:30Z

@jreback Rebased. Is any other thing required?

jreback · 2014-07-21T15:11:23Z

can you show an example of where this fails (in current master)
can you point to where Tick is calculated several times (currently)?

sinhrks · 2014-07-22T11:44:41Z

Affected offset (reset nanosecond)

If the aply logic includes datetime conversion, nanosecond will be lost.

CustomBusinessDay
CustomBusinessMonthEnd
CustomBusinessMonthBegin
MonthBegin
BusinessMonthBegin
MonthEnd
BusinessMonthEnd
YearBegin
BYearBegin
YearEnd
BYearEnd
QuarterBegin
BQuarterBegin

Affected offset (dateutil support)

tz.localize raises AttributeError: 'tzfile' object has no attribute 'localize' if tz is dateutil timezone.

BQuarterBegin
QuarterEnd
BQuarterEnd
LastWeekOfMonth
FY5253Quarter
FY5253
WeekOfMonth
Easter

sinhrks · 2014-07-22T11:48:57Z

can you point to where Tick is calculated several times (currently)?

Caused by these:

Check hasattr

https://github.com/pydata/pandas/blob/master/pandas/tslib.pyx#784

Twice again in nanosecond conversion (hasattr and actual addition)

https://github.com/pydata/pandas/blob/master/pandas/tslib.pyx#784

jreback · 2014-07-22T11:56:20Z

not sure what you mean (about Tick being calced more than once). The 2nd time is simply int_64 addition. AFAICT. delta_to_nanoseconds is necessary. What do you think this should change to?

sinhrks · 2014-07-22T12:22:43Z

Yeah all operations are necessary. What I meant is every time delta is being calculated, thus it may better to change cache_readonly
https://github.com/pydata/pandas/blob/master/pandas/tseries/offsets.py#L2016.

Actually this is not affects to performance so much, thus it is possible to leave it as normal property.

jreback · 2014-07-22T12:31:10Z

I think that delta and nanos could be cache_readonly. Once you create an offset they are not changed (I don't think). You can try that (but separate PR). I am not sure how to test that, maybe just trace the code and see.

jreback · 2014-07-22T12:31:53Z

@sinhrks otherwise this looks ok. It just changes a lot of code so trying to review.

sinhrks · 2014-07-22T12:44:30Z

OK, modified to normal property.

jreback · 2014-07-22T17:55:39Z

seems, apply_wraps is on every apply, except for in DateOffset. maybe add a note there why this is (or is it right?)

jreback · 2014-07-22T18:03:48Z

pandas/tseries/offsets.py

+                        value = result.value
+                    result = Timestamp(value + nano)
+
+            if tz is not None and result.tzinfo is None:


shouldn't this be result= tslib._localize_pydatetime(result, tz) as well here?

Because this is a flow for Timestamp, no need to care for datetime here.

hmm, maybe add a note (or you can simply use the other routine). I found it confusing?

OK. Modified to use tslib._localize_pydatetime(result, tz) to avoid any confusion.

sinhrks · 2014-07-24T11:46:21Z

DateOffset needs apply_wraps. I missed because of misunderstanding that DateOffset cannot be used by itself (#7375). Fixed and added tests.

jreback · 2014-07-24T13:34:22Z

ok, ping when green

sinhrks · 2014-07-24T14:02:50Z

@jreback now green.

BUG/PERF: offsets.apply doesnt preserve nanosecond

jreback · 2014-07-25T15:11:07Z

@sinhrks thanks for this...cleans up a large amount of code.....

jreback reviewed Jul 8, 2014
View reviewed changes

jreback added Bug labels Jul 8, 2014

jreback added this to the 0.15.0 milestone Jul 8, 2014

sinhrks mentioned this pull request Jul 9, 2014

ENH/BUG: Offset.apply dont preserve time #7375

Merged

sinhrks mentioned this pull request Jul 22, 2014

BUG: DateOffset weekday around DST produces unexpected results. fixes #5175 #5292

Merged

jreback reviewed Jul 22, 2014
View reviewed changes

BUG: offsets.apply doesnt preserve nanosecond

458ccf0

jreback added a commit that referenced this pull request Jul 25, 2014

Merge pull request #7697 from sinhrks/offsetnano

415fbfc

BUG/PERF: offsets.apply doesnt preserve nanosecond

jreback merged commit 415fbfc into pandas-dev:master Jul 25, 2014

sinhrks deleted the offsetnano branch July 25, 2014 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG/PERF: offsets.apply doesnt preserve nanosecond #7697

BUG/PERF: offsets.apply doesnt preserve nanosecond #7697

sinhrks commented Jul 8, 2014

jreback Jul 8, 2014

sinhrks Jul 9, 2014

sinhrks Jul 9, 2014

jreback commented Jul 21, 2014

sinhrks commented Jul 21, 2014

jreback commented Jul 21, 2014

sinhrks commented Jul 22, 2014

sinhrks commented Jul 22, 2014

jreback commented Jul 22, 2014

sinhrks commented Jul 22, 2014

jreback commented Jul 22, 2014

jreback commented Jul 22, 2014

sinhrks commented Jul 22, 2014

jreback commented Jul 22, 2014

jreback Jul 22, 2014

sinhrks Jul 24, 2014

jreback Jul 24, 2014

sinhrks Jul 24, 2014

sinhrks commented Jul 24, 2014

jreback commented Jul 24, 2014

sinhrks commented Jul 24, 2014

jreback commented Jul 25, 2014

BUG/PERF: offsets.apply doesnt preserve nanosecond #7697

BUG/PERF: offsets.apply doesnt preserve nanosecond #7697

Conversation

sinhrks commented Jul 8, 2014

Before

After the fix

jreback Jul 8, 2014

Choose a reason for hiding this comment

sinhrks Jul 9, 2014

Choose a reason for hiding this comment

sinhrks Jul 9, 2014

Choose a reason for hiding this comment

jreback commented Jul 21, 2014

sinhrks commented Jul 21, 2014

jreback commented Jul 21, 2014

sinhrks commented Jul 22, 2014

Affected offset (reset nanosecond)

Affected offset (dateutil support)

sinhrks commented Jul 22, 2014

jreback commented Jul 22, 2014

sinhrks commented Jul 22, 2014

jreback commented Jul 22, 2014

jreback commented Jul 22, 2014

sinhrks commented Jul 22, 2014

jreback commented Jul 22, 2014

jreback Jul 22, 2014

Choose a reason for hiding this comment

sinhrks Jul 24, 2014

Choose a reason for hiding this comment

jreback Jul 24, 2014

Choose a reason for hiding this comment

sinhrks Jul 24, 2014

Choose a reason for hiding this comment

sinhrks commented Jul 24, 2014

jreback commented Jul 24, 2014

sinhrks commented Jul 24, 2014

jreback commented Jul 25, 2014