Skip to content

BUG: PeriodIndex fails to handle NA, rather than putting NaT in its place (#46673) #47780

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

Merged
merged 11 commits into from
Aug 1, 2022
Merged

BUG: PeriodIndex fails to handle NA, rather than putting NaT in its place (#46673) #47780

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3b589b1
BUG: PeriodIndex fails to handle NA, rather than putting NaT in its p…
kapiliyer Jul 18, 2022
8ba96f2
BUG: PeriodIndex fails to handle NA, rather than putting NaT in its p…
kapiliyer Jul 18, 2022
9ff4708
Merge branch 'main' into bug_46673
kapiliyer Jul 18, 2022
f2c6723
BUG: Series.astype is unable to handle pd.nan.
kapiliyer Jul 21, 2022
3d9e157
BUG: PeriodIndex fails to handle NA, rather than putting NaT in its p…
kapiliyer Jul 22, 2022
c28fd8a
Merge branch 'main' into bug_46673
kapiliyer Jul 22, 2022
ab1d2a7
BUG: Series.astype is unable to handle pd.nan (#46377)
kapiliyer Jul 22, 2022
ca6c0fd
BUG: PeriodIndex fails to handle NA, rather than putting NaT in its p…
kapiliyer Jul 22, 2022
8e53d06
BUG: PeriodIndex fails to handle NA, rather than putting NaT in its p…
kapiliyer Jul 22, 2022
caa4a83
Merge branch 'main' into bug_46673
kapiliyer Jul 25, 2022
3458d3d
BUG: PeriodIndex fails to handle NA, rather than putting NaT in its p…
kapiliyer Jul 25, 2022
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.5.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ Indexing
- Bug in :meth:`NDFrame.xs`, :meth:`DataFrame.iterrows`, :meth:`DataFrame.loc` and :meth:`DataFrame.iloc` not always propagating metadata (:issue:`28283`)
- Bug in :meth:`DataFrame.sum` min_count changes dtype if input contains NaNs (:issue:`46947`)
- Bug in :class:`IntervalTree` that lead to an infinite recursion. (:issue:`46658`)
- Bug in :class:`PeriodIndex` raising ``AttributeError`` when indexing on ``NA``, rather than putting ``NaT`` in its place. (:issue:`46673`)
-

Missing
Expand Down
3 changes: 2 additions & 1 deletion pandas/_libs/tslibs/nattype.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ from numpy cimport int64_t
cnp.import_array()

cimport pandas._libs.tslibs.util as util
from pandas._libs.missing cimport C_NA
from pandas._libs.tslibs.np_datetime cimport (
get_datetime64_value,
get_timedelta64_value,
Expand Down Expand Up @@ -1217,7 +1218,7 @@ cdef inline bint checknull_with_nat(object val):
"""
Utility to check if a value is a nat or not.
"""
return val is None or util.is_nan(val) or val is c_NaT
return val is None or util.is_nan(val) or val is c_NaT or val is C_NA
Copy link
Member

Choose a reason for hiding this comment

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

@jbrockmendel I guess NaT is NA-like?

Copy link
Member

Choose a reason for hiding this comment

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

depends on context. NA has different semantics than NaN/NaT, so it isn't obvious we should accept it everywhere.

changing it here in particular changes behavior in a bunch of places

Copy link
Contributor Author

@kapiliyer kapiliyer Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jbrockmendel
Thank you for the input. I see how this function is used in a lot of different places, not just in converting to PeriodIndex which is the main thing I was trying to get at. I will try an approach that more directly tackles the specific case of #46673 (assuming that the expected behavior listed in the issue is valid, i.e.

Expected Behavior
The return value of
pd.PeriodIndex(["2022-04-06", "2022-04-07", pd.NA], freq='D')
should be the same as
pd.PeriodIndex(["2022-04-06", "2022-04-07", None], freq='D')
that is:
PeriodIndex(['2022-04-06', '2022-04-07', 'NaT'], dtype='period[D]')

Copy link
Member

Choose a reason for hiding this comment

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

assuming that the expected behavior listed in the issue is valid

Unfortunately it isn't entirely obvious that this should be the expected behavior. bc pd.NA has different semantics than pd.NaT, we might want to distinguish between them in this context and do some kind of NullableArray[Period]

This also would introduce an inconsistency between PeriodIndex vs the Period constructor, which is a whole other can of worms (and then we get to the same issues for Timestamp/Timedelta...)

Then again, I'm an outlier in crankiness on pd.NA-consistency-headaches, so if @mroeschke is fine with the proposed behavior, i won't make a stink about it.



cdef inline bint is_dt64nat(object val):
Expand Down
15 changes: 15 additions & 0 deletions pandas/tests/arrays/categorical/test_indexing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import math

import numpy as np
import pytest

from pandas import (
NA,
Categorical,
CategoricalIndex,
Index,
Interval,
IntervalIndex,
NaT,
PeriodIndex,
Series,
Timedelta,
Expand Down Expand Up @@ -194,6 +198,17 @@ def test_categories_assignments(self):
tm.assert_numpy_array_equal(cat.__array__(), exp)
tm.assert_index_equal(cat.categories, Index([1, 2, 3]))

@pytest.mark.parametrize(
"null_val",
[None, np.nan, NaT, NA, math.nan, "NaT", "nat", "NAT", "nan", "NaN", "NAN"],
)
def test_periodindex_on_null_types(self, null_val):
# GH 46673
result = PeriodIndex(["2022-04-06", "2022-04-07", null_val], freq="D")
expected = PeriodIndex(["2022-04-06", "2022-04-07", "NaT"], dtype="period[D]")
assert type(result[2]) == type(NaT)
tm.assert_index_equal(result, expected)

@pytest.mark.parametrize("new_categories", [[1, 2, 3, 4], [1, 2]])
def test_categories_assignments_wrong_length_raises(self, new_categories):
cat = Categorical(["a", "b", "c", "a"])
Expand Down
8 changes: 4 additions & 4 deletions pandas/tests/extension/test_arrow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,7 +1587,7 @@ def test_combine_le(self, data_repeated):

def test_combine_add(self, data_repeated, request):
pa_dtype = next(data_repeated(1)).dtype.pyarrow_dtype
if pa.types.is_temporal(pa_dtype):
if pa.types.is_temporal(pa_dtype) and pa_dtype != "duration[ns]":
request.node.add_marker(
pytest.mark.xfail(
raises=TypeError,
Expand Down Expand Up @@ -1697,7 +1697,7 @@ def test_arith_series_with_scalar(
elif arrow_temporal_supported:
request.node.add_marker(
pytest.mark.xfail(
raises=TypeError,
raises=TypeError if pa_dtype != "duration[ns]" else AssertionError,
reason=(
f"{all_arithmetic_operators} not supported between"
f"pd.NA and {pa_dtype} Python scalar"
Expand Down Expand Up @@ -1763,7 +1763,7 @@ def test_arith_frame_with_scalar(
elif arrow_temporal_supported:
request.node.add_marker(
pytest.mark.xfail(
raises=TypeError,
raises=TypeError if pa_dtype != "duration[ns]" else AssertionError,
reason=(
f"{all_arithmetic_operators} not supported between"
f"pd.NA and {pa_dtype} Python scalar"
Expand Down Expand Up @@ -1847,7 +1847,7 @@ def test_arith_series_with_array(
elif arrow_temporal_supported:
request.node.add_marker(
pytest.mark.xfail(
raises=TypeError,
raises=TypeError if pa_dtype != "duration[ns]" else AssertionError,
reason=(
f"{all_arithmetic_operators} not supported between"
f"pd.NA and {pa_dtype} Python scalar"
Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/series/methods/test_astype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ def test_astype_string_to_extension_dtype_roundtrip(
self, data, dtype, request, nullable_string_dtype
):
if dtype == "boolean" or (
dtype in ("period[M]", "datetime64[ns]", "timedelta64[ns]") and NaT in data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partially addresses #40566 by fixing handling of pd.NA for conversion to period types.

dtype in ("datetime64[ns]", "timedelta64[ns]") and NaT in data
):
mark = pytest.mark.xfail(
reason="TODO StringArray.astype() with missing values #GH40566"
Expand Down