BUG fixing module first() and DateOffset 
State: closed

- [ X] closes #45908 
- [ X] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [ ] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [ ] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

Also fixes part of #51284 (I have not worked on `last()`) 

~~Edge cases were only fixed for offsets passed as string.~~ 
~~Trying to make it work for `DateOffset` would make it more complicated. Should this be fixed or should `DateOffset` be removed from the function (and docs)?~~
~~Will `DataOffset` be [deprecated](https://github.com/pandas-dev/pandas/issues/48262) or fixed? it has bugs or the documentation is incorrect.~~

As commented by @MarcoGorelli:
* Functions `first()` and `last()` assume that the `Series` or `DataFrame` have a sorted `DatetimeIndex`. Only `last()` has it specified in the documentation. 

* `first()` could be easily done as follows (in case it's deprecated): 
`df[df.index < df.index[0] + DateOffset(days= 15)]`

I also tried  to use `first()` with `relativedelta` but I couldn’t make it work.

Edit:

`first()` doesn’t work for `DateOffset`. With this PR `DateOffset` works, but it now seems that there are inconsistencies when given a string or `MonthEnd`. 
I think that since the PRs to fix https://github.com/pandas-dev/pandas/issues/29623, `first()` with a string doesn’t work as normal offset arithmetic. 


Labels:
name: Frequency
description: DateOffsets
name: Stale

Comments:
> Functions first() and last() assume that the Series or DataFrame have a sorted DatetimeIndex. Only last() has it specified in the documentation.

do you want to start by making a docs PR to fix this? I think this would need doing regardless of whether `DateOffset` were supported
> do you want to start by making a docs PR to fix this?

Yes
> would it work to just do

No... look:
```
>>> x = pd.Series([1] * 10, index=pd.bdate_range("2010-03-31", periods=10))
>>> result = x.first(pd.DateOffset(months=1))
>>> result
2010-03-31    1
2010-04-01    1
2010-04-02    1
2010-04-05    1
2010-04-06    1
2010-04-07    1
2010-04-08    1
2010-04-09    1
2010-04-12    1
2010-04-13    1
Freq: B, dtype: int64
>>> result2 = x.first("1M")
>>> result2
2010-03-31    1
Freq: B, dtype: int64
```

It doesn't need to start with an "end of the month" to fail. 
The reason is that if offset is a DateOffset:
```
offset.is_on_offset(self.index[0])
True
```
If offset is string:
```
offset.is_on_offset(self.index[0])
False
```

We need to substract the `Timedelta(1, unit= ___)` - we could take the unit from the DateOffset.kwds or transform DateOffset to string, to start with. But I'm not sure if that's a good solution. 

Isn't that fine though?

Because `DateOffset(months=1)` is different from `MonthEnd(1)` (and `'M'`, somewhat confusingly, means 'month end' - but there is a PR open to deprecate and rename that)

```python
In [4]: pd.Timestamp.now() + pd.DateOffset(months=1)
Out[4]: Timestamp('2023-05-11 16:26:01.139005')

In [5]: pd.Timestamp.now() + pd.tseries.offsets.MonthEnd(1)
Out[5]: Timestamp('2023-04-30 16:26:14.195128')
```

I think it makes sense that
```
x.first(pd.DateOffset(months=1))
```
would take the rows within one calendar month of the first element, whereas `x.first('1M')` would only go until the first month-end
Oh, OK! 

> but there is a PR open to deprecate and rename that

Does that mean that both (string offset and `DateOffset`) will return the same answer eventually?

Should I add examples with `DateOffset` in the docs, and new tests maybe? 

`last()` will need to be fixed too

I don't need to speak @ the meeting, yaaay! 

> Does that mean that both (string offset and DateOffset) will return the same answer eventually?

Sorry I was unclear, what I meant was that `'M'` will be renamed to `'ME'` to make it clear that it means "month end" (rather than calendar month, like `DateOffset(months=1)`)

> Should I add examples with DateOffset in the docs, and new tests maybe?

Sounds good!
> Sorry I was unclear

No reason to apologise. Thanks for explaining. 

I will add examples in the docs on another PR if that's fine.

Would somebody very kind review this "annoying" PR @MarcoGorelli ? :)  
> Nice one, thanks! We can do a follow-up for `last` as well after

My pleasure... Yes, we can do `last()` as well.

It's unclear to me why you only commented about using `date_range` on one test, but not the others. I mean, I use it on the other tests as well.
And when I removed `date_range` from line 122 (`def test_first_with_date_offset`) the `assert_equal`  failed because `freq=None` on `expected`. So I removed it also from the input `i`. I’m not sure if that’s OK. 

Before I removed `freq`at the input, I tried adding it to `expected`, but the methods I tried used an offset as well.

@MarcoGorelli : it looks green  
Thanks @MarcoGorelli for the review (and the patience).
Friendly ping @MarcoGorelli @jbrockmendel 

> this introduces an inconsistency between `first('M')` and `first(MonthEnd())` (which should mean the same thing - reminder that 'M' means 'month end'):

I think the original issue #29623 was not a bug but `first()` (with a string) was modified to match the docstring: “For instance, ‘1M’ will display all the rows having their index within the first month.”

If `first` accepts a `DateOffset` then it should always handle this offset in a way which is consistent with `DateOffset` arithmetic. If the first timestamp in the index is '2020-01-31' then a MonthEnd() offset means going to the following month end:

```
from pandas.tseries.offsets import MonthEnd, Day
from pandas.tseries.frequencies import to_offset
from pandas.tseries.offsets import DateOffset

t = pd.Timestamp('2020-01-31')
t + MonthEnd()
Timestamp('2020-02-29 00:00:00’)
```
Adding for example 15 days means going 15 days forward

```
t + Day(15)
Timestamp('2020-02-15 00:00:00’)
```
Here we go to mid February with a 15-day offset. In this example, the timespan covered by an offset of month end is thus longer than that of an offset of 15 days. It would be completely unexpected for the user if the time period returned by `first` would be the opposite (ie shorter with a MonthEnd offset than with a 15-day offset). 

For example:

```
i = pd.date_range('2020/1/31', periods=31, freq="D")
s = pd.Series(range(len(i)), index=i)

print(s.first(MonthEnd()).tail(5))
2020-02-24    24
2020-02-25    25
2020-02-26    26
2020-02-27    27
2020-02-28    28
Freq: D, dtype: int64

print(s.first(DateOffset(months=1)).tail(5))
2020-02-24    24
2020-02-25    25
2020-02-26    26
2020-02-27    27
2020-02-28    28
Freq: D, dtype: int64

print(s.first(‘1M').tail(5))
2020-01-31    0
Freq: D, dtype: int64

print(s.first(‘15D').tail(5))
2020-02-10    10
2020-02-11    11
2020-02-12    12
2020-02-13    13
2020-02-14    14
Freq: D, dtype: int64
```

So I think the options should be the following :
- have `first()` behave with a DateOffset input with normal offset arithmetic and behave with a string input as it does now (what we have on this PR). But explain the difference in the docstring.
- allow only string inputs to avoid any confusion and any change in the current string input behaviour.
- change the behaviour with a string input so that it behaves consistently with month end semantics.  The docstring could be changed to that effect to :'1M' will display all the rows having their index  within the first month end following the first element in the index

I'm not sure if I have to (friendly) ping you :) @MarcoGorelli 
> I think the original issue https://github.com/pandas-dev/pandas/issues/29623 was not a bug but first() (with a string) was modified to match the docstring:

I'm inclined to agree, and am pretty tempted to suggest reverting the PR which changed it

I'll add it to Wednesday's agenda
This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
I will work on this shortly
Closing this one because `Series/DataFrame.first()` has been deprecated: https://github.com/pandas-dev/pandas/pull/53419