Skip to content

BUG: DataFrame.drop unexpectedly drops frequency #58846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

BUG: DataFrame.drop unexpectedly drops frequency #58846

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
823a7d6
BUG: DataFrame.drop unexpectedly drop frequency
mcmrc May 27, 2024
f279c61
modify to infer freq
mcmrc May 28, 2024
d8f2d98
fix pre-commit error
mcmrc May 28, 2024
f517609
override DatetimeIndex.drop
mcmrc Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v3.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ Categorical
Datetimelike
^^^^^^^^^^^^
- Bug in :class:`Timestamp` constructor failing to raise when ``tz=None`` is explicitly specified in conjunction with timezone-aware ``tzinfo`` or data (:issue:`48688`)
- Bug in :func:`Datafreme.drop` returning ``Freq=None`` when the dataframe has a ``DatetimeIndex`` (:issue:`58743`)
- Bug in :func:`date_range` where the last valid timestamp would sometimes not be produced (:issue:`56134`)
- Bug in :func:`date_range` where using a negative frequency value would not include all points between the start and end values (:issue:`56382`)
- Bug in :func:`tseries.api.guess_datetime_format` would fail to infer time format when "%Y" == "%H%M" (:issue:`57452`)
Expand Down
10 changes: 9 additions & 1 deletion pandas/core/indexes/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6949,7 +6949,15 @@ def drop(
if errors != "ignore":
raise KeyError(f"{labels[mask].tolist()} not found in axis")
indexer = indexer[~mask]
return self.delete(indexer)
new_index = self.delete(indexer)

# check if we need to set the freq attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing this here, override DatetimeIndex.drop

from pandas import DatetimeIndex

if isinstance(self, DatetimeIndex):
new_index.freq = self.freq
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m pretty sure this will not be correct in the general case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbrockmendel
Thank you so much for your quick review. Sorry that I totally misunderstood the case....
What about the code below inferring freq from the newly created DatetimeIndex. (cf. #22561)

        if isinstance(self, DatetimeIndex):
            new_index.freq = to_offset(new_index.inferred_freq)

Copy link
Author

@mcmrc mcmrc May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code above passed unit tests except for test_delete_slice2 in test_delete.py

test_delete_slice2 expects df.drop returns freq=None. But after this fix, expected values are like <2 * Days>, <2 * Years>, and so on. Should I revise test_delete_slice2 to use _with_freq('infer') instead?

        # reset freq to None
        result = ts.drop(ts.index[[1, 3, 5, 7, 9]]).index
        expected = dti[::2]._with_freq(None) # should be 'infer' instead
        tm.assert_index_equal(result, expected)
        assert result.name == expected.name
        assert result.freq == expected.freq
        assert result.tz == expected.tz


return new_index

@final
def infer_objects(self, copy: bool = True) -> Index:
Expand Down
16 changes: 16 additions & 0 deletions pandas/tests/generic/test_generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
deepcopy,
)

from datetime import datetime
import numpy as np
import pytest

Expand All @@ -12,6 +13,7 @@
DataFrame,
Index,
Series,
Timestamp,
date_range,
)
import pandas._testing as tm
Expand Down Expand Up @@ -483,3 +485,17 @@ def test_flags_identity(self, frame_or_series):
assert obj.flags is obj.flags
obj2 = obj.copy()
assert obj2.flags is not obj.flags

@pytest.mark.parametrize("freq", ["Y", "M", "D"])
def test_drop_method_freq_preservation(self, freq):
start = "1970-01-01"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a "# GH#???" reference

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added # GH 58846

index = date_range(start=start, periods=10, freq=freq)
df = DataFrame((np.ones(len(index))), index=index)

# set inplace as false
test_df = df.drop(index=df.index[0], inplace=False)
tm.assert_equal(test_df.index.freq, index.freq)

# set inplace as true
df.drop(index=df.index[0], inplace=True)
tm.assert_equal(df.index.freq, index.freq)
Loading