BUG: Fix a bug in 'timedelta_range' that produced an extra point on a edge case (fix #30353)

[hasB4K]

The issue from #30353 came actually from timedelta_range.

import pandas as pd

def mock_timedelta_range(start=None, end=None, periods=None, freq=None, name=None, closed=None):
    epoch = pd.Timestamp(0)
    if start is not None:
        start = epoch + pd.Timedelta(start)
    if end is not None:
        end = epoch + pd.Timedelta(end)
    res = pd.date_range(start=start, end=end, periods=periods, freq=freq, name=name, closed=closed)
    res -= epoch
    res.freq = freq
    return res

print(mock_timedelta_range("1day", "10day", freq="2D"))
print(pd.timedelta_range("1day", "10day", freq="2D"))

The outputs from mock_timedelta_range and from pd.timedelta_range are supposed to equivalent, but are not on pandas v1.0.0:
Outputs without this PR:

TimedeltaIndex(['1 days', '3 days', '5 days', '7 days', '9 days'], dtype='timedelta64[ns]', freq='2D')
TimedeltaIndex(['1 days', '3 days', '5 days', '7 days', '9 days', '11 days'], dtype='timedelta64[ns]', freq='2D')

Outputs with this PR:

TimedeltaIndex(['1 days', '3 days', '5 days', '7 days', '9 days'], dtype='timedelta64[ns]', freq='2D')
TimedeltaIndex(['1 days', '3 days', '5 days', '7 days', '9 days'], dtype='timedelta64[ns]', freq='2D')

---

It also solve an issue (that fail on master) related to this comment: #13022 (comment)

import pandas as pd
rng = pd.timedelta_range(start='1d', periods=10, freq='d')
df = pd.Series(range(10), index=rng)
df.resample('2D').count()

Outputs with this PR:

1 days    2
3 days    2
5 days    2
7 days    2
9 days    2
Freq: 2D, dtype: int64

---

On the firsts commits I just fixed the issue in _generate_regular_range, then I decided to do a refactor and to use the same code that generate ranges in core/arrays/_ranges.py for date_range and timedelta_range.

Linked with #10887.

---

• closes Resampler sometimes adds extra bin with NaN #30353
  closes BUG: resample with TimedeltaIndex, fenceposts are off #13022
  closes loffset has no effect when passing in a numyp.timedelta64 #7687 (for loffset when resampling Timedelta)
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @hasB4K! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-05-09 16:24:27 UTC

[WillAyd]

I think the refactor looks reasonable - @jbrockmendel any thoughts?

[jbrockmendel]

A few comments on the refactor, but i really like the idea of re-using generate_regular_range

[hasB4K]

I hope this is okay @WillAyd 🙂. I'll be happy to change this if you disagree with this changelog.

BTW, regarding loffset, as mentioned earlier (https://github.com/pandas-dev/pandas/pull/33498/files#r407231964):

Useful because otherwise tests/resample/test_base.py::test_resample_loffset_arg_type was always failing.
However, this piece of code is questionable since #31809 will deprecate loffset anyway.

[jbrockmendel]

so this is testing incorrect behavior? thats pretty weird

[hasB4K]

@jbrockmendel Yeah, that was the case. That was testing an incorrect behavior... 😕 And this was the case for two reasons:

• loffset support was missing in .resample when dealing with timedelta. (loffset has no effect when passing in a numyp.timedelta64 #7687)
• an extra bin (fixed with this PR) was present each time when dealing with timedelta. (BUG: resample with TimedeltaIndex, fenceposts are off #13022, Resampler sometimes adds extra bin with NaN #30353)

Here an example of the values when setting a breakpoint with PyCharm line 222 in pandas/tests/resample/test_base.py on master:

The expected result (with loffset and without an extra bin):

The actual result (without loffset and with an extra bin):

I think that was the reason of the presence of this TODO:

pandas/pandas/tests/resample/test_base.py

Line 218 in 3d4f9dc

# GH 13022, 7687 - TODO: fix resample w/ TimedeltaIndex

IMO, the presence of this if was just to skip the tests of Timedeltas when resampling without skipping the tests of Timestamps and Periods parametrized with @all_ts.

[jreback]

lgtm a couple of questions.

[hasB4K]

lgtm a couple of questions.

Thank you! 🙂
I have just answered on those and fixed the issue on the test you mentioned earlier.

Let me know if you need anything else!

[jreback]

thanks @hasB4K very nice!