Skip to content
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.

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

REGR: allow reindexing datetimelike with upcast / raise deprecation warning #44839

Merged
merged 15 commits into from
Jan 16, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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/v1.4.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@ Datetimelike
- Bug in adding a ``np.timedelta64`` object to a :class:`BusinessDay` or :class:`CustomBusinessDay` object incorrectly raising (:issue:`44532`)
- Bug in :meth:`Index.insert` for inserting ``np.datetime64``, ``np.timedelta64`` or ``tuple`` into :class:`Index` with ``dtype='object'`` with negative loc adding ``None`` and replacing existing value (:issue:`44509`)
- Bug in :meth:`Series.mode` with ``DatetimeTZDtype`` incorrectly returning timezone-naive and ``PeriodDtype`` incorrectly raising (:issue:`41927`)
- Fixed regression in :meth:`~Series.reindex` raising an error when using an incompatible fill value with a datetime-like dtype (or not raising a deprecation warning for using a ``datetime.date`` as fill value) (:issue:`42921`)
- Bug in :class:`DateOffset`` addition with :class:`Timestamp` where ``offset.nanoseconds`` would not be included in the result (:issue:`43968`, :issue:`36589`)
- Bug in :meth:`Timestamp.fromtimestamp` not supporting the ``tz`` argument (:issue:`45083`)
- Bug in :class:`DataFrame` construction from dict of :class:`Series` with mismatched index dtypes sometimes raising depending on the ordering of the passed dict (:issue:`44091`)
Expand Down
6 changes: 6 additions & 0 deletions pandas/core/array_algos/take.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ def take_nd(
"""
if fill_value is lib.no_default:
fill_value = na_value_for_dtype(arr.dtype, compat=False)
elif isinstance(arr.dtype, np.dtype) and arr.dtype.kind in "mM":
Copy link
Member

Choose a reason for hiding this comment

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

does take_1d need to be changed to match?

dtype, fill_value = maybe_promote(arr.dtype, fill_value)
if arr.dtype != dtype:
# EA.take is strict about returning a new object of the same type
# so for that case cast upfront
arr = arr.astype(dtype)

if not isinstance(arr, np.ndarray):
# i.e. ExtensionArray,
Expand Down
2 changes: 1 addition & 1 deletion pandas/core/dtypes/cast.py
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ def _maybe_promote(dtype: np.dtype, fill_value=np.nan):
except (ValueError, TypeError):
pass
else:
if fv.tz is None:
if isna(fv) or fv.tz is None:
return dtype, fv.asm8

return np.dtype("object"), fill_value
Expand Down
8 changes: 8 additions & 0 deletions pandas/tests/frame/methods/test_reindex.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import numpy as np
import pytest

import pandas.util._test_decorators as td

import pandas as pd
from pandas import (
Categorical,
Expand Down Expand Up @@ -97,6 +99,7 @@ def test_reindex_copies(self):
result2 = df.reindex(columns=cols, index=df.index, copy=True)
assert not np.shares_memory(result2[0]._values, df[0]._values)

@td.skip_array_manager_not_yet_implemented
def test_reindex_date_fill_value(self):
# passing date to dt64 is deprecated
arr = date_range("2016-01-01", periods=6).values.reshape(3, 2)
Expand All @@ -113,6 +116,11 @@ def test_reindex_date_fill_value(self):
)
tm.assert_frame_equal(res, expected)

# only reindexing rows
with tm.assert_produces_warning(FutureWarning):
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this a warn?

Copy link
Member

Choose a reason for hiding this comment

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

xref #44798 (comment).

@jorisvandenbossche maybe open dedicated issue for discussion/visibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this PR can serve that purpose?

Copy link
Member

Choose a reason for hiding this comment

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

could use a match=... to clarify

res = df.reindex(index=range(4), fill_value=fv)
tm.assert_frame_equal(res, expected[["A", "B"]])

# same with a datetime-castable str
res = df.reindex(
index=range(4), columns=["A", "B", "C"], fill_value="2016-01-01"
Expand Down
17 changes: 17 additions & 0 deletions pandas/tests/series/methods/test_reindex.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
Period,
PeriodIndex,
Series,
Timedelta,
Timestamp,
date_range,
isna,
)
Expand Down Expand Up @@ -300,6 +302,21 @@ def test_reindex_fill_value():
tm.assert_series_equal(result, expected)


@pytest.mark.parametrize("dtype", ["datetime64[ns]", "timedelta64[ns]"])
@pytest.mark.parametrize("fill_value", ["string", 0, Timedelta(0)])
def test_reindex_fill_value_datetimelike_upcast(dtype, fill_value):
# https://github.com/pandas-dev/pandas/issues/42921
if dtype == "timedelta64[ns]" and fill_value == Timedelta(0):
# use the scalar that is not compatible with the dtype for this test
fill_value = Timestamp(0)

ser = Series([NaT], dtype=dtype)

result = ser.reindex([0, 1], fill_value=fill_value)
expected = Series([None, fill_value], index=[0, 1], dtype=object)
tm.assert_series_equal(result, expected)


def test_reindex_datetimeindexes_tz_naive_and_aware():
# GH 8306
idx = date_range("20131101", tz="America/Chicago", periods=7)
Expand Down