ENH: add 'origin' and 'offset' arguments to 'resample' and 'pd.Grouper'
State: closed

**EDIT:** this PR has changed, now instead of adding `adjust_timestamp` we are adding `origin` and `offset` arguments to `resample` and `pd.Grouper` (see https://github.com/pandas-dev/pandas/pull/31809#issuecomment-583884772)

----

Hello,

This enhancement is an alternative to the `base` argument present in `pd.Grouper` or in the method `resample`. It adds the `adjust_timestamp` argument to change the current behavior of: https://github.com/pandas-dev/pandas/blob/master/pandas/core/resample.py#L1728

-  `adjust_timestamp` is the timestamp on which to adjust the grouping. If None is passed, the first day of the time series at midnight is used.

Currently the bins of the grouping are adjusted based on the beginning of the day of the time series starting point. This works well with frequencies that are multiples of a day (like `30D`) or that divides a day (like `90s` or `1min`). But it can create inconsistencies with some frequencies that do not meet this criteria.

Here is a simple snippet from a test that I added that proves that the current behavior can lead to some inconsistencies. Inconsistencies that can be fixed if we use `adjust_timestamp`:

```python
import pandas as pd
import numpy as np
import pandas._testing as tm
import pytest


freq = "1399min"  # prime number that is smaller than 24h
start, end = "1/1/2000 00:00:00", "1/31/2000 00:00"
middle = "1/15/2000 00:00:00"

rng = pd.date_range(start, end, freq="1231min")  # prime number
ts = pd.Series(np.random.randn(len(rng)), index=rng)
ts2 = ts[middle:end]

# proves that grouper without a fixed adjust_timestamp does not work
# when dealing with unusual frequencies
simple_grouper = pd.Grouper(freq=freq)
count_ts = ts.groupby(simple_grouper).agg("count")
count_ts = count_ts[middle:end]
count_ts2 = ts2.groupby(simple_grouper).agg("count")
with pytest.raises(AssertionError):
    tm.assert_index_equal(count_ts.index, count_ts2.index)

# test adjusted_timestamp on 1970-01-01 00:00:00
adjust_timestamp = pd.Timestamp(0)
adjusted_grouper = pd.Grouper(freq=freq, adjust_timestamp=adjust_timestamp)
adjusted_count_ts = ts.groupby(adjusted_grouper).agg("count")
adjusted_count_ts = adjusted_count_ts[middle:end]
adjusted_count_ts2 = ts2.groupby(adjusted_grouper).agg("count")
tm.assert_series_equal(adjusted_count_ts, adjusted_count_ts2)
```

I think this PR is ready to be merged, but I am of course open to any suggestions or criticism. :wink: 
For instance, I am not sure if the naming of `adjust_timestamp` is correct. An alternative could be `base_timestamp` or `ref_timestamp` :thinking:?

Cheers,

----

- [X] closes #25226
closes #28302
closes #28675
closes #4197
closes #8521
- [X] Add 'origin' and 'offset' arguments to 'resample' and 'pd.Grouper'
- [X] tests added / passed
- [X] Add deprecation warning for `loffset` and `base` in the code
- [X] Add deprecation warning for `loffset` and `base` in the doc
- [x] Add examples in the doc for `origin` and `offset`
- [x] whatsnew entry (add deprecation notice with `offset` example)
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Resample
description: resample method
name: API - Consistency
description: Internal Consistency of API/Behavior

Comments:
Hello @hasB4K! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



























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

##### Comment last updated at 2020-05-09 21:51:23 UTC
Instead of adding a new keyword, might be nice if `base` could take a `Timestamp` instead since they are both relevant when a frequency is passed.
I would rather suggest the following:

I always thought that the `base` argument has kind of an ambiguous name. And the current behavior is quite confusing. It needs to be an integer (or a floating point) that matches the unit of the frequency:
- So `base=1` with a frequency of `5D` is equal to `1D` that we add as an offset.
- So `base=2` with a frequency of `5min` is equal to `2min` that we add as an offset.

This behavior is very confusing for the users (myself included), but it also creates bugs: see #25161, #25226

Instead of relying on `base` I would rather deprecate this argument. The argument `loffset` (currently broken for `pd.Grouper` as shown in #28302, but fixable in the current PR) is kind of equivalent to what `base` is doing (especially since it is a Timedelta).

**Example of the current use of loffset with resample:**
```python
>>> start, end = "1/1/2000 00:00:00", "1/31/2000 00:00"
>>> rng = pd.date_range(start, end, freq="1231min")
>>> ts = pd.Series(np.arange(len(rng)), index=rng)
>>> ts.resample("1min", loffset=-pd.Timedelta("1min")).count()
```
```
1999-12-31 23:59:00    1
2000-01-01 00:00:00    0
2000-01-01 00:01:00    0
2000-01-01 00:02:00    0
2000-01-01 00:03:00    0
                      ..
2000-01-30 22:00:00    0
2000-01-30 22:01:00    0
2000-01-30 22:02:00    0
2000-01-30 22:03:00    0
2000-01-30 22:04:00    1
Freq: T, Length: 43086, dtype: int64
```

**Example of the current broken loffset argument:**
```python
>>> ts.groupby(pd.Grouper(freq="1min", loffset=-pd.Timedelta("1min"))).count()
```
```
2000-01-01 00:00:00    1
2000-01-01 00:01:00    0
2000-01-01 00:02:00    0
2000-01-01 00:03:00    0
2000-01-01 00:04:00    0
                      ..
2000-01-30 22:01:00    0
2000-01-30 22:02:00    0
2000-01-30 22:03:00    0
2000-01-30 22:04:00    0
2000-01-30 22:05:00    1
Freq: T, Length: 43086, dtype: int64
```

That being said, I agree that the naming of adjust_timestamp is not ideal. I would rename it into: `origin` or `base_timestamp`.

The line https://github.com/pandas-dev/pandas/blob/master/pandas/core/resample.py#L1728 would be replaced by something roughly equivalent to:
```
origin = start_of_day if origin is None else start_of_day
origin = origin.value + loffset.value
```

TL;DR:

1. I would fix in this PR `loffset` for pd.Grouper and deprecate the confusing `base` argument
2. I would rename the added argument `adjust_timestamp` into `origin`
3. We would have `origin = origin.value + loffset.value`
4. I would add more tests to check the behavior of `loffset` and `origin`

What do you think?
I just realised that `loffset` and `base` are not equivalent at all since this works:

```python
>>> start, end = "1/1/2000 00:00:00", "1/31/2000 00:00"
>>> rng = pd.date_range(start, end, freq="1231min")
>>> ts = pd.Series(np.arange(len(rng)), index=rng)
>>> ts.resample("1min", loffset=-pd.Timedelta("365D")).count()
```
```
1999-01-01 00:00:00    1
1999-01-01 00:01:00    0
1999-01-01 00:02:00    0
1999-01-01 00:03:00    0
1999-01-01 00:04:00    0
                      ..
1999-01-30 22:01:00    0
1999-01-30 22:02:00    0
1999-01-30 22:03:00    0
1999-01-30 22:04:00    0
1999-01-30 22:05:00    1
Freq: T, Length: 43086, dtype: int64
```

So I would suggest the following instead:

1. Deprecate the confusing `base` argument
2. Add an `origin_offset` argument that would be a Timestamp
3. Rename the added argument `adjust_timestamp` into `origin_timestamp`
4. We would have `origin_nanos = origin_timestamp.value + origin_offset.value`

I will not fix `loffset` in this PR since I am not sure of the behavior with `pd.Grouper` and how to fix it.

What do you think ?
@jreback this won't fix the issue that I'm trying to tackle. The idea is to be able to have a **fixed timestamp as a "origin"** that does not depend of the time series. So neither the base argument with `first` (which is the current behavior) or `last` string will fix the issue.

I could use the `base` argument and use it as the "origin" argument that I want to add if `base`is not a number like suggested @mroeschke. But I think this could create some confusion in the API (I still believe that `base` is useful but can be quite confusing to use).
> @jreback this won't fix the issue that I'm trying to tackle. The idea is to be able to have a **fixed timestamp as a "origin"** that does not depend of the time series. So neither the base argument with `first` (which is the current behavior) or `last` string will fix the issue.
> 
> I could use the `base` argument and use it as the "origin" argument that I want to add if `base`is not a number like suggested @mroeschke. But I think this could create some confusion in the API (I still believe that `base` is useful but can be quite confusing to use).

@hasB4K not averse with changing things. But we currently have base, loffset, so I don' really like the idea of another another pretty opaque options. 

I would be onboard with deprecating both of these and replacing with 2 options, e.g. origin and offset come to mind.
> I would be onboard with deprecating both of these and replacing with 2 options, e.g. origin and offset come to mind.

So would this signature be ok with you @jreback?
```
pd.Grouper(
    # ... arguments untouched with this enhancement like `freq`, `sort`, ...

    # ADDED arguments
    origin: pd.Timestamp, default None
        Only when freq is passed.
        The timestamp used as reference for the grouping bins.
        If None is passed, the first day of the time series at midnight is used.
    offset: pd.Timedelta, default None
        Only when freq is passed.
        An offset timedelta added to the origin.

    # DEPRECATED arguments
    base: int, default 0 (DEPRECATED)
    loffset: str, DateOffset, timedelta object (DEPRECATED)
)
```

@hasB4K 

sure that looks reasonable.

we would need to have a pretty nice deprecation message that shows one how to convert base and/or loffset to the new args (as well as a whatsnew and warning box in the docs); they can bascially be the same though. its how we want folks to migrate.
> sure that looks reasonable.

Perfect, I will implement that in this PR then :slightly_smiling_face: 

> we would need to have a pretty nice deprecation message that shows one how to convert base and/or loffset to the new args (as well as a whatsnew and warning box in the docs); they can bascially be the same though. its how we want folks to migrate.

Yep, it seems quite necessary! Is there an example of a nice deprecation message in the current (or in the old) code that I could look into?
For now, I was thinking of adding to the documentation of `resample` and `pd.Grouper` examples of "how to migrate". And in the code something like `this argument is deprecated, please see: <url>`.
> Yep, it seems quite necessary! Is there an example of a nice deprecation message in the current (or in the old) code that I could look into?
> For now, I was thinking of adding to the documentation of `resample` and `pd.Grouper` examples of "how to migrate". And in the code something like `this argument is deprecated, please see: <url>`.

there are some (recently removed in 1.0.0) deprecation messages in resample on how to handle the freq arg. myabe not *great* but ok :->



@jreback I still need to add more examples for 'origin' and 'offset' and update the "what's new" part of the doc, but otherwise, it's ready for review :slightly_smiling_face: 

@jreback I have changed the main code issues, I haven't changed the documentation yet. I'll do that probably tomorrow if I have some time :slightly_smiling_face:
@jreback The tests are passing, the doc has been updated, and I have fixed the issues in the code that you pinpointed. I think this PR is ready :slightly_smiling_face: 
By the way, there is some flaky tests not related to my PR (see https://github.com/pandas-dev/pandas/pull/32031). It appears since I have rebased with master.

The second reason why the CI can fail (at least that's the case for the last few hours for what I have seen) comes from `continuous-integration/travis-ci/pr`. It seems that if fails when trying to upload the coverage :thinking: 

As shown here:
https://travis-ci.org/pandas-dev/pandas/jobs/651140009#L2423
```
= 61718 passed, 749 skipped, 603 xfailed, 7 xpassed, 211 warnings in 1238.98s (0:20:38) =
uploading coverage
bash <(curl -s https://codecov.io/bash) -Z -c -f /tmp/test_coverage.xml
/dev/fd/63: line 1: html: No such file or directory
/dev/fd/63: line 2: syntax error near unexpected token `<'
'
```

----
**EDIT:** I just rebased, again, with master, and everything seems to work just fine :slightly_smiling_face:. However, I still don't know why the coverage failed at the upload stage :sweat_smile:.
needs a rebase as well.

can you look thru open resample tests (I may have asked this already), and see if any examples are out there that this PR would close, if so pls add them as examples.
cc @mroeschke if you'd have a look.
Looks like there's a diff with `doc/source/whatsnew/v1.0.0.rst` although it probably shouldn't have been touched. Could you double check the diff?
> Looks like there's a diff with `doc/source/whatsnew/v1.0.0.rst` although it probably shouldn't have been touched. Could you double check the diff?

I just removed the 'execute' permission on this file, for some reason, this file was set as "executable". I thought that was a mistake since all others rst files didn't have this permission flag.
Sorry for the delay, I have now more time to work on that PR :)
I think I have addressed all the points that needed changes.

There is still the weird code on how I handle the warning for both `df.resample` and `pd.Grouper`. I explain more of this in these comments:
https://github.com/pandas-dev/pandas/pull/31809#discussion_r379861081 (full explanation of the issue)
https://github.com/pandas-dev/pandas/pull/31809#discussion_r387602206 (the follow-up comment)
https://github.com/pandas-dev/pandas/pull/31809#discussion_r387600949 (link of the tests of the warnings)
> Looks like `doc/source/whatsnew/v1.0.0.rst` changed in the diff. Could you revert it?

I removed the change. The only change I made there was `chmod -x v1.0.0.rst` because, for some reason, this is the only 'whatsnew' doc file with the 'execute flag'. But anyway, this is not important :wink:
> I removed the change. The only change I made there was chmod -x v1.0.0.rst because, for some reason, this is the only 'whatsnew' doc file with the 'execute flag'. But anyway, this is not important 😉

Ah I see. Happy to have that change in another PR
I have fixed the main issue with the warning, even though I think this is clearly not a perfect solution.

Here a list of the last 3 ongoing discussions:
*  https://github.com/pandas-dev/pandas/pull/31809#discussion_r403509397 The solution to warn properly with the `stacklevel` "challenge"
* https://github.com/pandas-dev/pandas/pull/31809#discussion_r403510811 The confusion of what happened with the deprecated `loffset` argument (and the tests associated with it)
* https://github.com/pandas-dev/pandas/pull/31809#discussion_r403393179 The confusing example with `origin` and epoch
would it make sense for origin=‘epoch’ to possible? maybe even a future default?
> would it make sense for origin=‘epoch’ to possible? maybe even a future default?

Yes it would, however setting that as a default would be a breaking change for frequencies that does not divide or is not a multiple of a day.

Currently `origin=0` works for setting epoch. Do you want me to add `'epoch'` as a possible value of `origin`?
> would it make sense for origin=‘epoch’ to possible? maybe even a future default?

After investigating #28675 and #8521, I think your comment raises a good remark. I think we should let `origin` be equal to:
* a timestamp (the current implementation)
* the string `'epoch'` which would be equal to the epoch (with a UTC tz)
* the string `'start'` which would set `origin` to the start of the timeseries (in `core.resample::_adjust_dates_anchored`, `origin` would be set to `first`) - it would solve issues like #28675 and #8521 more easily.
* the string `'start_day'` which would set the `origin` to the start of the day of the series. (the current default behavior)

I think we should let the default to `start_day` since not doing so would be a breaking change.

@jreback what do you think?
i think those are reasonable values

can you be sure the doc-string and examples show when to use 
> i think those are reasonable values
> 
> can you be sure the doc-string and examples show when to use

@jreback I have added the support of `'epoch'`, `'start'` and `'start_day'` to the argument `origin` with its docs, examples and tests :slightly_smiling_face:. It should be ready for review.
Just checking in, do I need to add or fix anything else @jreback? :slightly_smiling_face: 
@mroeschke if you can have a look.
note that we are likey to merge: https://github.com/pandas-dev/pandas/pull/33498 first, so will need a rebase after.