TST: add datetimelike tests for tz-aware DatetimeIndex #17694
Conversation
This commit just makes quick patches to get tests working for subclassing common index tests for DatetimeIndex, by either ignoring tests entirely or converting to the correct TZ.
|
(Most of the CI builds are failing, but as far as I can tell they are miscellaneous errors unrelated to this PR.) |
sinhrks
added
Datetime
| @@ -34,6 +34,12 @@ def verify_pickle(self, index): | |||
| unpickled = tm.round_trip_pickle(index) | |||
| assert index.equals(unpickled) | |||
|
|
|||
| def _fix_tz(self, new_index, orig_index): | |||
| if hasattr(orig_index, 'tz'): | |||
There was a problem hiding this comment.
really don't spell things like this. rather change create_index() to work properly.
| def _fix_tz(self, new_index, orig_index): | ||
| if hasattr(orig_index, 'tz'): | ||
| assert new_index.tz is None | ||
| new_index = new_index.tz_localize('UTC').tz_convert(orig_index.tz) |
There was a problem hiding this comment.
create a new class DatetimeTZ in test_datetimelike.py which also inherits Datetimelike. Then just change its create_index
There was a problem hiding this comment.
To clarify, this commit already introduces a new Datetimelike-derived class in test_datetimelikely.py with create_index() overriden to return a tz-aware DatetimeIndex: test_datetimelike.py#L159. This _fix_tz hack here is not to create the underlying index to be tested, but to "fix" various transformations of the index which don't preserve TZ, to get them to compare correctly back to the output of create_index(). But like you mentioned in the comment below, we can move more of this logic to class members instead.
| @@ -278,6 +284,8 @@ def test_ensure_copied_data(self): | |||
|
|
|||
| index_type = index.__class__ | |||
| result = index_type(index.values, copy=True, **init_kwargs) | |||
| result = self._fix_tz(result, index) | |||
There was a problem hiding this comment.
you don't need to touch this because _holder will also be different.
We shold prob refactor this init_kwargs bizness to instead be a class method, that way each class can construct things easily
IOW
class .....:
# e.g. for PeriodIndex
_init_kwargs = lambda index: return {'freq': index.freq'}
# in the base class (no-op)
_init_kwargs = lambda index: return {}
There was a problem hiding this comment.
_holder here will also be DatetimeIndex, which isn't sufficient because DatetimeIndex constructor doesn't have TZ information. The problem is that the constructor DatetimeIndex(index.values, ...) is getting a numpy datetime64 array of tz-naive timestamps. There currently doesn't seem to be an argument to DatetimeIndex's constructor to tell it what timezone to convert to presuming that the input data is UTC (the tz= argument localizes to the specified timezone), so there's not currently a way to do this through init_kwargs. Hence why I currently call _fix_tz to first localize to UTC and then convert to the TZ of create_index().
To sum up:
index = create_index()hasTZ = US/Easternresult = index_type(index.values)hasTZ = None❌result = index_type(index.values, tz=index.tz)hasTZ = US/Eastern, but because of re-localization has been shifted again, and thus has differently underlying values. ❌self._fix_tz(result, index)does haveTZ = US/Eastern
I can do the suggested refactor though.
There was a problem hiding this comment.
yes prob need to refactor a bit,
|
closing as stale, but opened an issue to track |
This commit just makes quick patches to get tests working for subclassing common index tests for tz-aware DatetimeIndex, by either ignoring tests entirely or explicitly converting to the DatetimeIndex's TZ. This is split off from PR #17583 where we were trying to add another common test. Advice is requested for whether to try to also make fixes upstream or just to keep these fixes localized to the test suite.
git diff upstream/master -u -- "*.py" | flake8 --diff