cleanup; use timedelta instead of relativedelta where possible #18183

jbrockmendel · 2017-11-09T04:48:46Z

Cleanup, flake8, modernize imports
Replace usage of other + relativedelta(...) with other + timedelta(...) or other.replace(...) where possible. Clearer and more performant.
Use _get_firstbday in one place that had previously duplicated its logic.

asv continuous -f 1.1 -E virtualenv master HEAD -b timeseries
[...]
       before           after         ratio
     [8dac6331]       [c6145385]
-      31.4±0.2μs       26.7±0.3μs     0.85  timeseries.Offsets.time_custom_bday_decr
-      14.4±0.1ms       12.0±0.2ms     0.84  timeseries.DatetimeIndex.time_infer_freq_none
-           1.76s            1.45s     0.82  timeseries.Iteration.time_iter_periodindex
-      53.8±0.2μs       42.2±0.2μs     0.78  timeseries.SemiMonthOffset.time_end_decr_n
-        53.1±4μs       41.1±0.3μs     0.78  timeseries.SemiMonthOffset.time_begin_incr_n
-      54.6±0.2μs       41.5±0.2μs     0.76  timeseries.SemiMonthOffset.time_begin_decr_n
-      3.55±0.5ms      2.70±0.01ms     0.76  timeseries.ToDatetime.time_iso8601_nosep
-      51.8±0.2μs       38.6±0.2μs     0.75  timeseries.SemiMonthOffset.time_begin_decr
-        53.0±3μs       38.1±0.3μs     0.72  timeseries.SemiMonthOffset.time_end_incr_n
-      47.5±0.3μs       34.0±0.3μs     0.72  timeseries.SemiMonthOffset.time_end_incr
-      49.2±0.2μs       34.9±0.8μs     0.71  timeseries.SemiMonthOffset.time_begin_incr
-      55.8±0.1μs         39.3±1μs     0.70  timeseries.SemiMonthOffset.time_end_decr
-      43.6±0.5μs       30.7±0.2μs     0.70  timeseries.SemiMonthOffset.time_begin_apply
-      43.6±0.4μs       30.6±0.1μs     0.70  timeseries.SemiMonthOffset.time_end_apply

pep8speaks · 2017-11-09T04:48:48Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on November 09, 2017 at 04:50 Hours UTC

codecov · 2017-11-09T05:34:52Z

Codecov Report

Merging #18183 into master will decrease coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master   #18183      +/-   ##
==========================================
- Coverage   91.42%    91.4%   -0.02%     
==========================================
  Files         163      163              
  Lines       50068    50065       -3     
==========================================
- Hits        45776    45764      -12     
- Misses       4292     4301       +9

Flag	Coverage Δ
#multiple	`89.21% <92.85%> (-0.01%)`	⬇️
#single	`40.36% <14.28%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`97.1% <92.85%> (-0.01%)`	⬇️
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 8dac633...ce75d62. Read the comment docs.

codecov · 2017-11-09T05:34:53Z

Codecov Report

Merging #18183 into master will decrease coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master   #18183      +/-   ##
==========================================
- Coverage   91.42%    91.4%   -0.02%     
==========================================
  Files         163      163              
  Lines       50068    50065       -3     
==========================================
- Hits        45776    45764      -12     
- Misses       4292     4301       +9

Flag	Coverage Δ
#multiple	`89.21% <92.85%> (-0.01%)`	⬇️
#single	`40.36% <14.28%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`97.1% <92.85%> (-0.01%)`	⬇️
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 8dac633...ce75d62. Read the comment docs.

codecov · 2017-11-09T05:34:56Z

Codecov Report

Merging #18183 into master will decrease coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master   #18183      +/-   ##
==========================================
- Coverage   91.42%    91.4%   -0.02%     
==========================================
  Files         163      163              
  Lines       50068    50065       -3     
==========================================
- Hits        45776    45764      -12     
- Misses       4292     4301       +9

Flag	Coverage Δ
#multiple	`89.21% <92.85%> (-0.01%)`	⬇️
#single	`40.36% <14.28%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`97.1% <92.85%> (-0.01%)`	⬇️
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 8dac633...ce75d62. Read the comment docs.

jorisvandenbossche · 2017-11-09T12:45:10Z

Question (it has been a long time I worked with relativedeltas): in the non-replace case (so when using plural kwargs to relativedelta), is addition with relativedelta then always fully equivalent to addition of a timedelta?
Eg there are no differences in how they handle DST changes? (where adding 1 day might actually add 24 hours but change the hour of the result, or keep the hour of the result and thus adds 23/25 hours)

jreback · 2017-11-09T12:51:07Z

pandas/tseries/offsets.py

@@ -2382,7 +2379,7 @@ def apply(self, other):
                    qtr_lens = self.get_weeks(other + self._offset)

                for weeks in qtr_lens:
-                    start += relativedelta(weeks=weeks)


i would leave the weeks, not real well tested for timedelta; relativedelta may be more intelligent here

i would leave the weeks, not real well tested for timedelta

Not sure I understand. You mean the stdlib timedelta isn't well-tested?

relativedelta may be more intelligent here

The relativedelta constructor just adds weeks*7 to the days kwarg.

jbrockmendel · 2017-11-09T16:16:56Z

Question (it has been a long time I worked with relativedeltas):

The fact that relativedelta's behavior is more obscure is part of why this change is helpful. (Also the perf improvement is a free lunch)

in the non-replace case (so when using plural kwargs to relativedelta), is addition with relativedelta then always fully equivalent to addition of a timedelta?

For everything weeks and shorter, yes. Months and years have more complicated behavior.

Eg there are no differences in how they handle DST changes?

In the pertinent case, relativedelta.__add__ passes through to timedelta:

ret = (other.replace(**repl)
               + datetime.timedelta(days=days,
                                    hours=self.hours,
                                    minutes=self.minutes,
                                    seconds=self.seconds,
                                    microseconds=self.microseconds))

jbrockmendel · 2017-11-09T19:46:16Z

Working on fixes for #8386 and a few related things. That should hurt perf very slightly. I'd like to get these perf improvements in so as to stay "in the black". (and to keep the cleanup separate from the non-trivial upcoming diffs)

jbrockmendel · 2017-11-10T20:43:01Z

Superceded by #18218.

cleanup; use timedelta instead of relativedelta where possible

c614538

whitespace fixup

ce75d62

jreback requested changes Nov 9, 2017

View reviewed changes

jreback added Frequency DateOffsets Code Style Code style, linting, code_checks labels Nov 9, 2017

jreback mentioned this pull request Nov 10, 2017

Implement scalar shift_month mirroring tslib.shift_months #18218

Merged

jbrockmendel closed this Nov 10, 2017

jbrockmendel deleted the tslibs-offsets7 branch November 12, 2017 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup; use timedelta instead of relativedelta where possible #18183

cleanup; use timedelta instead of relativedelta where possible #18183

jbrockmendel commented Nov 9, 2017

pep8speaks commented Nov 9, 2017 •

edited

Loading

codecov bot commented Nov 9, 2017

codecov bot commented Nov 9, 2017

codecov bot commented Nov 9, 2017 •

edited

Loading

jorisvandenbossche commented Nov 9, 2017

jreback Nov 9, 2017

jbrockmendel Nov 9, 2017

jbrockmendel Nov 9, 2017

jbrockmendel commented Nov 9, 2017

jbrockmendel commented Nov 9, 2017

jbrockmendel commented Nov 10, 2017

cleanup; use timedelta instead of relativedelta where possible #18183

cleanup; use timedelta instead of relativedelta where possible #18183

Conversation

jbrockmendel commented Nov 9, 2017

pep8speaks commented Nov 9, 2017 • edited Loading

Comment last updated on November 09, 2017 at 04:50 Hours UTC

codecov bot commented Nov 9, 2017

Codecov Report

codecov bot commented Nov 9, 2017

Codecov Report

codecov bot commented Nov 9, 2017 • edited Loading

Codecov Report

jorisvandenbossche commented Nov 9, 2017

jreback Nov 9, 2017

Choose a reason for hiding this comment

jbrockmendel Nov 9, 2017

Choose a reason for hiding this comment

jbrockmendel Nov 9, 2017

Choose a reason for hiding this comment

jbrockmendel commented Nov 9, 2017

jbrockmendel commented Nov 9, 2017

jbrockmendel commented Nov 10, 2017

pep8speaks commented Nov 9, 2017 •

edited

Loading

codecov bot commented Nov 9, 2017 •

edited

Loading