BUG: fix inserting tz-aware datetime to Series, closes #12862 #27322
Conversation
| @@ -523,7 +523,22 @@ def test_setitem_with_tz_dst(): | |||
| tm.assert_series_equal(s, exp) | |||
|
|
|||
|
|
|||
| def test_categorial_assigning_ops(): | |||
| def test_setitem_new_key_tz(): | |||
There was a problem hiding this comment.
there is a file for timezones and timeseries tests under indexing
prefer one of those
There was a problem hiding this comment.
sure. for next time, when does a test go in tests.indexing vs tests.series.indexing vs tests.frame.test_indexing?
There was a problem hiding this comment.
both tests.series.indexing.test_datetime and tests.indexing.test_datetime are about objects with DatetimeIndex, not datetime values
There was a problem hiding this comment.
well test_indexing is really big, just like to split out when dealing with specific types (may not be perfect but that's the idea); this should clearly be there.
There was a problem hiding this comment.
i think figuring this out merits a dedicated pass(es) similar to tests.arithmetic and tests.reductions
There was a problem hiding this comment.
see my comment below, I am sure we test this for .loc, so this belongs with it.
There was a problem hiding this comment.
OK. Do you have a suggestion as to where that might be? I'm not eager to manually sort through 50k+ tests. (and there aren't any foo.loc setitem calls in either tests.series.test_timezones or tests.series.test_timeseries)
There was a problem hiding this comment.
test_loc_coerceion in pandas/tests/indexing/test_loc.py
There was a problem hiding this comment.
everything in that file is testing loc. you want to mix non-loc tests in with that?
pandas/core/series.py
Outdated
| @@ -1230,6 +1230,13 @@ def setitem(key, value): | |||
| if _is_unorderable_exception(e): | |||
| raise IndexError(key) | |||
|
|
|||
| if is_scalar(key) and not is_integer(key) and key not in self.index: | |||
There was a problem hiding this comment.
i would trace further
you are adding a new case whereby it’s likely an existing path is just incorrect somewhere
There was a problem hiding this comment.
the path here is above when we call self._set_with_engine(key, value), _set_with_engine calls self.index._engine.set_value(self._values, key, value). The engine expects self._values to be an ndarray, but here it is a DatetimeArray. Eventually it might be worth addressing this at the low-level engine level, but definitely out of scope for now.
There was a problem hiding this comment.
FWIW, that's where I got stuck trying to fix this same bug. I tried converting it to an ndarray before passing it into the engine but a lot of other routines failed.
There was a problem hiding this comment.
move this to _set_with and its better (after the cases are narrowed down)
jreback
added
Bug
Indexing
| pd.to_datetime(42).tz_localize("UTC"), | ||
| pd.to_datetime(666).tz_localize("UTC"), | ||
| ] | ||
|
|
There was a problem hiding this comment.
can you also test using .loc which give the same result
There was a problem hiding this comment.
we may actually have a test for this already (so good to combine)
pandas/core/series.py
Outdated
| @@ -1230,6 +1230,13 @@ def setitem(key, value): | |||
| if _is_unorderable_exception(e): | |||
| raise IndexError(key) | |||
|
|
|||
| if is_scalar(key) and not is_integer(key) and key not in self.index: | |||
There was a problem hiding this comment.
move this to _set_with and its better (after the cases are narrowed down)
| @@ -523,7 +523,22 @@ def test_setitem_with_tz_dst(): | |||
| tm.assert_series_equal(s, exp) | |||
|
|
|||
|
|
|||
| def test_categorial_assigning_ops(): | |||
| def test_setitem_new_key_tz(): | |||
There was a problem hiding this comment.
test_loc_coerceion in pandas/tests/indexing/test_loc.py
|
comments addressed i think |
|
ok thanks; this looks better. |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff