Skip to content

Fix DatetimeIndex.insert(pd.NaT) for tz-aware index #18883

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 8 commits into from
Dec 29, 2017
Merged

Fix DatetimeIndex.insert(pd.NaT) for tz-aware index #18883

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
83e7105
Fix insert NaT into tzaware datetime index
jbrockmendel Dec 20, 2017
5a469d2
Whatsnew note
jbrockmendel Dec 20, 2017
664c800
edit whatsnew, test several null types
jbrockmendel Dec 21, 2017
85ba78c
edits per reviewer requests
jbrockmendel Dec 21, 2017
3d429df
Merge branch 'master' of https://github.com/pandas-dev/pandas into in…
jbrockmendel Dec 21, 2017
a1df0f7
Merge branch 'master' of https://github.com/pandas-dev/pandas into in…
jbrockmendel Dec 24, 2017
31e3581
fix typo
jbrockmendel Dec 24, 2017
8934f66
parametrize more
jbrockmendel Dec 29, 2017
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/v0.22.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ Indexing
- Bug in tz-aware :class:`DatetimeIndex` where addition/subtraction with a :class:`TimedeltaIndex` or array with ``dtype='timedelta64[ns]'`` was incorrect (:issue:`17558`)
- :func:`Index.to_series` now accepts ``index`` and ``name`` kwargs (:issue:`18699`)
- :func:`DatetimeIndex.to_series` now accepts ``index`` and ``name`` kwargs (:issue:`18699`)
- Bug in :func:`DatetimeIndex.insert` where inserting ``NaT`` into a timezone-aware index incorrectly raised (:issue:`16357`)

I/O
^^^
Expand Down
3 changes: 2 additions & 1 deletion pandas/core/indexes/datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1776,7 +1776,8 @@ def insert(self, loc, item):

if isinstance(item, (datetime, np.datetime64)):
self._assert_can_do_op(item)
if not self._has_same_tz(item):
if not self._has_same_tz(item) and item is not self._na_value:
Copy link
Contributor

@jreback jreback Dec 21, 2017

Choose a reason for hiding this comment

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

notnull(item)

Copy link
Member Author

@jbrockmendel jbrockmendel Dec 21, 2017

Choose a reason for hiding this comment

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

Will change, but FYI they are equivalent here. notnull(item) was checked above in which case it was replaced with self._na_value.

# GH#16537 allow pd.NaT through
Copy link
Contributor

@jreback jreback Dec 21, 2017

Choose a reason for hiding this comment

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

comment not needed

Copy link
Member Author

@jbrockmendel jbrockmendel Dec 21, 2017

Choose a reason for hiding this comment

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

Will change, but sure we don't want the issue reference?

raise ValueError(
'Passed item and index have different timezone')
# check freq can be preserved on edge cases
Expand Down
16 changes: 9 additions & 7 deletions pandas/tests/indexes/datetimes/test_indexing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ def test_where_tz(self):
expected = i2
tm.assert_index_equal(result, expected)

@pytest.mark.parametrize('tz', [None, 'UTC', 'US/Eastern'])
def test_insert_nat(self, tz):
Copy link
Member

@jschendel jschendel Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You could maybe parametrize over different NA values here, since this should work for inserting np.nan and None as well (shouldn't require any additional code changes). For example, on a tz-naive DTI:

In [2]: pd.date_range('20170101', periods=2).insert(0, np.nan)
Out[2]: DatetimeIndex(['NaT', '2017-01-01', '2017-01-02'], dtype='datetime64[ns]', freq=None)

In [3]: pd.date_range('20170101', periods=2).insert(0, None)
Out[3]: DatetimeIndex(['NaT', '2017-01-01', '2017-01-02'], dtype='datetime64[ns]', freq=None)

Copy link
Member

@jschendel jschendel Dec 21, 2017

Choose a reason for hiding this comment

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

If you do that, I suppose you could delete these lines in test_insert below, as to not duplicate things:

pandas/pandas/tests/indexes/datetimes/test_indexing.py

Lines 148 to 153 in ac8ac15

# GH 18295 (test missing)
expected = DatetimeIndex(
['20170101', pd.NaT, '20170102', '20170103', '20170104'])
for na in (np.nan, pd.NaT, None):
result = date_range('20170101', periods=4).insert(1, na)
tm.assert_index_equal(result, expected)

# GH#16537, GH#18295 (test missing)
idx = pd.DatetimeIndex(['2017-01-01'], tz=tz)
expected = pd.DatetimeIndex(['NaT', '2017-01-01'], tz=tz)
for null in [None, np.nan, pd.NaT]:
Copy link
Contributor

@jreback jreback Dec 29, 2017

Choose a reason for hiding this comment

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

can you pull this null into the paramaterization?

Copy link
Member Author

@jbrockmendel jbrockmendel Dec 29, 2017

Choose a reason for hiding this comment

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

Sure, pushing momentarily.

res = idx.insert(0, null)
tm.assert_index_equal(res, expected)

def test_insert(self):
idx = DatetimeIndex(
['2000-01-04', '2000-01-01', '2000-01-02'], name='idx')
Expand Down Expand Up @@ -145,13 +154,6 @@ def test_insert(self):
assert result.tz == expected.tz
assert result.freq is None

# GH 18295 (test missing)
expected = DatetimeIndex(
['20170101', pd.NaT, '20170102', '20170103', '20170104'])
for na in (np.nan, pd.NaT, None):
result = date_range('20170101', periods=4).insert(1, na)
tm.assert_index_equal(result, expected)

def test_delete(self):
idx = date_range(start='2000-01-01', periods=5, freq='M', name='idx')

Expand Down