Repository: pandas-dev/pandas
Pull request: BUG: first("1M") returning two months when first day is last day of month
- [x] closes #29623
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
BUG: incorrect output of first('1M') in case if first index is the last day of the month
#### Code Sample

```python
x = pd.Series(1, index=pd.bdate_range('2010-03-31', periods=100))
print(x.first('1M'))  # Returns March and April
print(x.first('2M'))  # Returns March, April and May
# etc...
```
#### Problem description

In case if the first index of the series falls on the last day of the month, `Series.first('1M')` returns two first months (this one day + the next month).

If the first value is not on the last day of the month - result is correct:

```python
x = pd.Series(1, index=pd.bdate_range('2010-03-30', periods=100))
print(x.first('1M'))  # Returns only March
print(x.first('2M'))  # Returns March and April
# etc...
```

#### Output of ``pd.show_versions()``

<details>


INSTALLED VERSIONS
------------------
commit           : None
python           : 3.7.1.final.0
python-bits      : 64
OS               : Darwin
OS-release       : 18.7.0
machine          : x86_64
processor        : i386
byteorder        : little
LC_ALL           : en_US.UTF-8
LANG             : en_US.UTF-8
LOCALE           : en_US.UTF-8

pandas           : 0.25.3
numpy            : 1.17.2
pytz             : 2019.3
dateutil         : 2.8.0
pip              : 19.3.1
setuptools       : 41.4.0
Cython           : None
pytest           : 5.2.1
hypothesis       : None
sphinx           : 2.2.0
blosc            : None
feather          : None
xlsxwriter       : None
lxml.etree       : 4.4.1
html5lib         : None
pymysql          : 0.9.3
psycopg2         : None
jinja2           : 2.10.1
IPython          : 7.8.0
pandas_datareader: None
bs4              : 4.6.3
bottleneck       : None
fastparquet      : 0.1.6
gcsfs            : None
lxml.etree       : 4.4.1
matplotlib       : 3.0.1
numexpr          : 2.7.0
odfpy            : None
openpyxl         : None
pandas_gbq       : None
pyarrow          : 0.12.1
pytables         : None
s3fs             : 0.3.4
scipy            : 1.3.1
sqlalchemy       : 1.3.8
tables           : 3.4.4
xarray           : None
xlrd             : 1.1.0
xlwt             : None
xlsxwriter       : None


</details>

Hints from issue discussion:
take
@jbrockmendel mind taking a look? That looks like a bug to me too, but may be we're missing something.

@rachel-frenkel let us know if you need help.
It looks like in `first` before setting `end_date = end = self.index[0] + offset` there needs to be a check for `offset.onOffset(index[0])`, in which case we should return `self.loc[:index[0]]`

Commit message: Improve if condition

Patch to review:
diff --git a/doc/source/whatsnew/v1.2.0.rst b/doc/source/whatsnew/v1.2.0.rst
index ac930b3e77785..49ea516b2bfad 100644
--- a/doc/source/whatsnew/v1.2.0.rst
+++ b/doc/source/whatsnew/v1.2.0.rst
@@ -590,6 +590,7 @@ Datetimelike
 - Bug in :meth:`Series.isin` with ``datetime64[ns]`` dtype and :meth:`.DatetimeIndex.isin` failing to consider timezone-aware and timezone-naive datetimes as always different (:issue:`35728`)
 - Bug in :meth:`Series.isin` with ``PeriodDtype`` dtype and :meth:`PeriodIndex.isin` failing to consider arguments with different ``PeriodDtype`` as always different (:issue:`37528`)
 - Bug in :class:`Period` constructor now correctly handles nanoseconds in the ``value`` argument (:issue:`34621` and :issue:`17053`)
+- Bug in :meth:`DataFrame.first` and :meth:`Series.first` returning two months for offset one month when first day is last calendar day (:issue:`29623`)
 
 Timedelta
 ^^^^^^^^^
diff --git a/pandas/core/generic.py b/pandas/core/generic.py
index 4a9e020a0fe46..ff5645072699e 100644
--- a/pandas/core/generic.py
+++ b/pandas/core/generic.py
@@ -8426,7 +8426,10 @@ def first(self: FrameOrSeries, offset) -> FrameOrSeries:
             return self
 
         offset = to_offset(offset)
-        end_date = end = self.index[0] + offset
+        if offset._day_opt == "end" and offset.is_on_offset(self.index[0]):
+            end_date = end = self.index[0]
+        else:
+            end_date = end = self.index[0] + offset
 
         # Tick-like, e.g. 3 weeks
         if isinstance(offset, Tick):
diff --git a/pandas/tests/frame/methods/test_first_and_last.py b/pandas/tests/frame/methods/test_first_and_last.py
index d21e1eee54e16..4ee7016004137 100644
--- a/pandas/tests/frame/methods/test_first_and_last.py
+++ b/pandas/tests/frame/methods/test_first_and_last.py
@@ -3,7 +3,7 @@
 """
 import pytest
 
-from pandas import DataFrame
+from pandas import DataFrame, bdate_range
 import pandas._testing as tm
 
 
@@ -69,3 +69,13 @@ def test_last_subset(self, frame_or_series):
 
         result = ts[:0].last("3M")
         tm.assert_equal(result, ts[:0])
+
+    @pytest.mark.parametrize("start, periods", [("2010-03-31", 1), ("2010-03-30", 2)])
+    def test_first_with_first_day_last_of_month(self, frame_or_series, start, periods):
+        # GH#29623
+        x = frame_or_series([1] * 100, index=bdate_range(start, periods=100))
+        result = x.first("1M")
+        expected = frame_or_series(
+            [1] * periods, index=bdate_range(start, periods=periods)
+        )
+        tm.assert_equal(result, expected)


Code review comments:
diff_hunk: @@ -8426,7 +8426,10 @@ def first(self: FrameOrSeries, offset) -> FrameOrSeries:
             return self
 
         offset = to_offset(offset)
-        end_date = end = self.index[0] + offset
+        if offset._day_opt == "end" and offset.is_on_offset(self.index[0]):
path: pandas/core/generic.py
text: @user1:
Could we use rollforward?

@author:
Instead of the if-else? Then no, This would work for offsets like 1M, but not for something like 10 Days. That is also the thing which lead me to putting ``offset._day_opt == "end"`` in. But we could use it like

```
        if offset._day_opt == "end":
            end_date = end = offset.rollforward(self.index[0])
        else:
            end_date = end = self.index[0] + offset
```

if this would be preferrably

@user1:
this is an OK solution, but ideally there should be a way to do this with only public methods.

> but not for something like 10 Days

can you give an example? (possibly merits a test so i dont try to incorrectly simplify this somewhere down the line)  if its only for Tick offsets, special-casing wouldn't be that bad since we already special case them a few lines down

@author:
Tick works, thanks.

``test_first_subset`` in `` pandas/tests/frame/methods/test_first_and_last.py `` covers the behavior and avoids the simplification (as I learned myself a bit earlier :))