Fix DatetimeIndex.insert(pd.NaT) for tz-aware index #18883
Conversation
| @@ -46,6 +46,14 @@ 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): | |||
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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
doc/source/whatsnew/v0.22.0.txt
Outdated
| @@ -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 :class:`DatetimeIndex.insert` where inserting ``pd.NaT`` into a timezone-aware index incorrectly raised (:issue:`16357`) | |||
There was a problem hiding this comment.
I think :class: should be :func: or :meth: instead, and it looks like previous convention has been to just use ``NaT`` with the pd. omitted.
jreback
changed the title
DatetimeIndex.insert(pd.NaT) for tz-aware index
jreback
added
Bug
Missing-data
pandas/core/indexes/datetimes.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
Will change, but FYI they are equivalent here. notnull(item) was checked above in which case it was replaced with self._na_value.
pandas/core/indexes/datetimes.py
Outdated
| @@ -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: | |||
| # GH#16537 allow pd.NaT through | |||
There was a problem hiding this comment.
Will change, but sure we don't want the issue reference?
Codecov Report
@@ Coverage Diff @@
## master #18883 +/- ##
==========================================
- Coverage 91.59% 91.56% -0.03%
==========================================
Files 150 150
Lines 48959 48967 +8
==========================================
- Hits 44843 44836 -7
- Misses 4116 4131 +15
Continue to review full report at Codecov.
|
|
Easy win without whatsnew conflicts. |
| # 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]: |
There was a problem hiding this comment.
can you pull this null into the paramaterization?
There was a problem hiding this comment.
Sure, pushing momentarily.
|
ping. |
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff