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

closes #5175

Wound up working on this in preparation for the DST change next week. Here's a first pass at a solution and a test.


Labels:
name: Bug
name: Timezones
description: Timezone data dtype

Comments:
the failed build is python 2.6; click on the travis link, then the failed one, scroll down and you can see the test failures.

timedelta doesn't have the total_seconds attribute in 2.6, so you can prob just change the test to compute it

e.g. instead of t.total_seconds() 

use t.hour_3600+t.minute_60+t.seconds....which will work under all python versions

@adamgreenhall can you rebase

how's this coming?

@adamgreenhall can you rebase this?

@adamgreenhall can you rebase?

@jreback - sorry this took so long to get around to - will try to get this done this week. Current status: rebased but many of the offsets tests are failing. 

Some of the tests unrelated to DST are failing because of a years kwd. Any ideas? Example:

```
ERROR: test_calendar (pandas.tseries.tests.test_offsets.TestCustomBusinessDay)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/adam/code/pandas/pandas/tseries/tests/test_offsets.py", line 596, in test_calendar
    assertEq(CDay(calendar=calendar), dt, datetime(2014, 1, 21))
  File "/Users/adam/code/pandas/pandas/tseries/offsets.py", line 504, in __init__
    holidays = kwds['calendar'].holidays()
  File "/Users/adam/code/pandas/pandas/tseries/holiday.py", line 245, in holidays
    rule_holidays = rule.dates(start, end, return_name=True)
  File "/Users/adam/code/pandas/pandas/tseries/holiday.py", line 119, in dates
    year_offset = DateOffset(years=1)
  File "/Users/adam/code/pandas/pandas/tseries/offsets.py", line 124, in __init__
    self._offset = timedelta(**kwds)
TypeError: 'years' is an invalid keyword argument for this function
```

ok - have my tests on DST passing - just the year offsets are failing now.

@jreback - tests are passing - all looks good.

can you check performance on this?

`test.perf.sh -b master -t HEAD` and post anything >1.2 ratio

also need a release note in Bug Fix section

Will work on the tests tomorrow.

Looks like a few of the performance metrics are taking longer. I imagine that `frame_ctor_dtindex_BDay` and `frame_ctor_dtindex_BMonthEnd` are definitely related to the changes. Likely [this line](https://github.com/pydata/pandas/pull/5292/files#diff-63e1050a658278e30a3e7a744c4d6435R150). 

```
groupby_first_float32                        |   5.6330 |   4.5720 |   1.2321 |
groupby_last                                 |   5.7027 |   4.5710 |   1.2476 |
frame_ctor_dtindex_BMonthEnd(2)              |   1.7046 |   1.3630 |   1.2506 |
replace_fillna                               |   2.6340 |   2.0750 |   1.2694 |
index_from_series_ctor                       |   0.0297 |   0.0233 |   1.2765 |
datetime_index_intersection                  |   0.4674 |   0.3633 |   1.2863 |
series_drop_duplicates_int                   |   1.1303 |   0.8393 |   1.3467 |
frame_reindex_axis1                          | 240.2460 | 177.1216 |   1.3564 |
stats_rolling_mean                           |   1.2903 |   0.9397 |   1.3731 |
groupby_multi_different_functions            |  19.0346 |  13.5396 |   1.4058 |
frame_ctor_dtindex_BDay(2)                   |   1.9636 |   1.3007 |   1.5097 |
frame_boolean_row_select                     |   0.6274 |   0.3480 |   1.8027 |
frame_from_records_generator_nrows           |   1.9564 |   1.0613 |   1.8434 |
```

yeah relativedelta is slow (that's why its not used unless you readlly need it).

I would refactor this to make the setup method callable, and then only call it when you really need to.

Is it relativedelta that is slow? that is what is [being used on master](https://github.com/pydata/pandas/blob/master/pandas/tseries/offsets.py#L113) for all computations. Or is it just the [additional timezone conversion](https://github.com/pydata/pandas/pull/5292/files#diff-63e1050a658278e30a3e7a744c4d6435R147)?

iirc relativedelta is only used for month/year offsets

not day or less

It was actually the [timezone conversion](https://github.com/pydata/pandas/pull/5292/files#diff-63e1050a658278e30a3e7a744c4d6435R150) that was taking a long time. I refactored a bit and now have things running faster (I think). 

But one of the custom date offset tests is now failing:

```
FAIL: test_datetimindex (pandas.tseries.tests.test_offsets.TestCustomBusinessMonthEnd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/adam/code/pandas/pandas/tseries/tests/test_offsets.py", line 754, in test_datetimindex
    datetime(2012,4,30))
AssertionError: Timestamp('2012-05-31 00:00:00', offset='CBM') != datetime.datetime(2012, 4, 30, 0, 0)
```

Any ideas? I'm not sure exactly what is going on in this test.

Also, the `test_perf.sh` script is returning inconsistent results between runs. How do you decide what increases are real and because of the changes you made?

Run 1 (>1.2x):

```
datetime_index_intersection                  |   0.6840 |   0.5631 |   1.2148 |
frame_xs_row                                 |   0.0550 |   0.0439 |   1.2514 |
groupby_sum_booleans                         |   1.8830 |   1.4087 |   1.3368 |
stats_rolling_mean                           |   1.2867 |   0.9611 |   1.3388 |
timeseries_large_lookup_value                |   0.0463 |   0.0320 |   1.4467 |
frame_object_unequal                         |   0.0126 |   0.0076 |   1.6562 |
```

Run 2  (>1.2x):

```
dataframe_resample_mean_string               |   2.7219 |   2.2534 |   1.2079 |
frame_assign_timeseries_index                |   1.2950 |   1.0667 |   1.2141 |
timeseries_large_lookup_value                |   0.0473 |   0.0387 |   1.2218 |
frame_reindex_columns                        |   0.5391 |   0.4320 |   1.2478 |
frame_xs_mi_ix                               |   0.5357 |   0.4030 |   1.3293 |
reindex_multiindex                           |   2.9380 |   2.2040 |   1.3330 |
panel_shift                                  |   0.1361 |   0.0920 |   1.4784 |
frame_object_unequal                         |   0.0120 |   0.0080 |   1.4950 |
frame_nonunique_unequal                      |   0.0176 |   0.0116 |   1.5205 |
frame_ctor_dtindex_QuarterEnd(2)             |   1.0840 |   0.6154 |   1.7616 |
groupby_sum_booleans                         |   2.2246 |   1.2584 |   1.7678 |
```

rebase on master, that test is fixed

thanks - rebased and all tests passing.

so again, why are you not using hours/minutes/seconds etc using `timedelta`?

you need to move the tests as I indicated above, so that all offsets are tested. The way you have it only a very limited number are.

hours/minutes/seconds/... now use `timedelta`

```
In [2]: pd.DateOffset(hours=3)._offset
Out[2]: datetime.timedelta(0, 10800)
```

I'll fix those tests.

I know we went around and around on this....but can you rebase?

Rebased. Sorry - this fell off my todo list. I think you wanted to change around the test structure. Looking back at it now - I'm not sure that there are many existing classes that would inherit a `TestDST` style class. Currently the DST corrections in this PR only apply to `DateOffset` objects (not `Ticks`). Thoughts?

I think this is tied to #7418, so needs to be moved to 0.15

@adamgreenhall I know we kept pushing this....sorry about that...

can you rebase and can revist (I think let's just put it in regardless of #7418)

cc @rockg 
cc @sinhrks

@adamgreenhall can you rebase? 

@jreback - rebased.

thanks. can you add a release note in v0.15.0.txt. this is a bug fix? and pls squash

@rockg @sinhrks pls have a look

@jreback @sinhrks @rockg - addressed comments, added some tests, added to release notes, rebased, and squashed. Want to have another look?

fixed some py2.6 and py3 compatibility issues, rerunning tests.

I was having some issues with nanoseconds in other versions of python/numpy. I wound up taking nanoseconds out of the DST tests (in hindsight it seemed like overkill to have them there). All tests are passing now. @jreback @sinhrks - can you take a look?

any updates on this? need anything from me?