-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
Make _freq/freq/tz/_tz/dtype/_dtype/offset/_offset all inherit reliably #24517
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.
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
Changes from all commits
e48023b
288255c
b0fb964
ae414de
469d91c
c8bd451
1456b6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,7 +210,7 @@ class DatetimeArrayMixin(dtl.DatetimeLikeArrayMixin, | |
# Constructors | ||
|
||
_attributes = ["freq", "tz"] | ||
_tz = None | ||
_dtype = None # type: Union[np.dtype, DatetimeTZDtype] | ||
_freq = None | ||
|
||
@classmethod | ||
|
@@ -231,8 +231,13 @@ def _simple_new(cls, values, freq=None, tz=None): | |
result = object.__new__(cls) | ||
result._data = values | ||
result._freq = freq | ||
tz = timezones.maybe_get_tz(tz) | ||
result._tz = timezones.tz_standardize(tz) | ||
if tz is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it cheap to detect when you need to call maybe_get_tz & tz_standarize? ideally these could be called in the DatetimeTZDtype constructor / alternatively could have a dedicate constructor,
|
||
dtype = _NS_DTYPE | ||
else: | ||
tz = timezones.maybe_get_tz(tz) | ||
tz = timezones.tz_standardize(tz) | ||
dtype = DatetimeTZDtype('ns', tz) | ||
result._dtype = dtype | ||
return result | ||
|
||
def __new__(cls, values, freq=None, tz=None, dtype=None, copy=False, | ||
|
@@ -399,9 +404,7 @@ def dtype(self): | |
If the values are tz-aware, then the ``DatetimeTZDtype`` | ||
is returned. | ||
""" | ||
if self.tz is None: | ||
return _NS_DTYPE | ||
return DatetimeTZDtype('ns', self.tz) | ||
return self._dtype | ||
|
||
@property | ||
TomAugspurger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def tz(self): | ||
|
@@ -411,10 +414,10 @@ def tz(self): | |
Returns | ||
------- | ||
datetime.tzinfo, pytz.tzinfo.BaseTZInfo, dateutil.tz.tz.tzfile, or None | ||
Returns None when the array is tz-naive. | ||
Returns None when the array is tz-naive. | ||
""" | ||
# GH 18595 | ||
return self._tz | ||
return getattr(self._dtype, "tz", None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason this uses the private attribute instead of the dtype property? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, feel free to change |
||
|
||
@tz.setter | ||
def tz(self, value): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -321,11 +321,10 @@ def _simple_new(cls, values, name=None, freq=None, tz=None, dtype=None): | |
|
||
dtarr = DatetimeArray._simple_new(values, freq=freq, tz=tz) | ||
result = object.__new__(cls) | ||
result._data = dtarr._data | ||
result._freq = dtarr.freq | ||
result._tz = dtarr.tz | ||
result._eadata = dtarr | ||
result.name = name | ||
# For groupby perf. See note in indexes/base about _index_data | ||
# TODO: make sure this is updated correctly if edited | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This TODO is for _index_data? In theory that shouldn't happen, since DatetimeIndex is immutable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In _libs.reduction there is a line:
which makes me wary. Is this just never relevant for DTI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the intent of all this, to directly mutate the buffer in place. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TomAugspurger I think all your other comments have been addressed, not sure about this one. Should this TODO comment be removed? Some other action taken? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this todo isn't necessary AFAICT. Also, I really don't think we should have inverted the relationship between the eadata and data attributes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just because that change isn't going to last through #24024, so I think it was unnecessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can't have both 1) _eadata being a property that depends on self.freq and 2) freq be a property that depends on _eadata. Definitely agree that 24024 should return things to the old pattern. |
||
result._index_data = result._data | ||
result._reset_identity() | ||
return result | ||
|
@@ -345,19 +344,6 @@ def _values(self): | |
else: | ||
return self.values | ||
|
||
@property | ||
def tz(self): | ||
# GH 18595 | ||
return self._tz | ||
|
||
@tz.setter | ||
def tz(self, value): | ||
# GH 3746: Prevent localizing or converting the index by setting tz | ||
raise AttributeError("Cannot directly set timezone. Use tz_localize() " | ||
"or tz_convert() as appropriate") | ||
|
||
tzinfo = tz | ||
|
||
@property | ||
def size(self): | ||
# TODO: Remove this when we have a DatetimeTZArray | ||
|
@@ -416,15 +402,18 @@ def __setstate__(self, state): | |
data = np.empty(nd_state[1], dtype=nd_state[2]) | ||
np.ndarray.__setstate__(data, nd_state) | ||
|
||
freq = own_state[1] | ||
tz = timezones.tz_standardize(own_state[2]) | ||
dtarr = DatetimeArray._simple_new(data, freq=freq, tz=tz) | ||
|
||
self.name = own_state[0] | ||
self._freq = own_state[1] | ||
self._tz = timezones.tz_standardize(own_state[2]) | ||
|
||
else: # pragma: no cover | ||
data = np.empty(state) | ||
np.ndarray.__setstate__(data, state) | ||
dtarr = DatetimeArray(data) | ||
|
||
self._data = data | ||
self._eadata = dtarr | ||
self._reset_identity() | ||
|
||
else: | ||
|
@@ -502,7 +491,9 @@ def union(self, other): | |
else: | ||
result = Index.union(this, other) | ||
if isinstance(result, DatetimeIndex): | ||
result._tz = timezones.tz_standardize(this.tz) | ||
# TODO: we shouldn't be setting attributes like this; | ||
# in all the tests this equality already holds | ||
result._eadata._dtype = this.dtype | ||
if (result.freq is None and | ||
(this.freq is not None or other.freq is not None)): | ||
result.freq = to_offset(result.inferred_freq) | ||
|
@@ -530,11 +521,12 @@ def union_many(self, others): | |
if this._can_fast_union(other): | ||
this = this._fast_union(other) | ||
else: | ||
tz = this.tz | ||
dtype = this.dtype | ||
this = Index.union(this, other) | ||
if isinstance(this, DatetimeIndex): | ||
this._tz = timezones.tz_standardize(tz) | ||
|
||
# TODO: we shouldn't be setting attributes like this; | ||
# in all the tests this equality already holds | ||
this._eadata._dtype = dtype | ||
return this | ||
|
||
def _can_fast_union(self, other): | ||
|
@@ -1129,9 +1121,20 @@ def slice_indexer(self, start=None, end=None, step=None, kind=None): | |
# Wrapping DatetimeArray | ||
|
||
@property | ||
def _eadata(self): | ||
return DatetimeArray._simple_new(self._data, | ||
tz=self.tz, freq=self.freq) | ||
def _data(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that Actually... how does this even work? If you don't have a setter (which I don't see) then simple_new should fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see you set eadata. It doesn't really matter since we're removing it soon anyway, but I'd prefer that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's right. In this PR we set _eadata inside _simple_new and make _data a property returning _eadata._data. The freq/tz passthrough doesn't work with _eadata as a property (as in master), so the only question is whether to also set _data in _simple_new. I chose to make it a property to prevent any shenanigans where they become untied. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think https://github.com/pandas-dev/pandas/pull/24024/files#diff-26a6d2ca7adfca586aabbb1c9dd8bf36R74 is what we want for eadata & freq (and if we can do it here, instead of that PR, then that's best). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why's that? I suppose I could see why setting doesn't work, since IIUC we create a new DateteimArray on each invocation of Should we just hold off on these changes til #24024 then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, of course it won't work, since we call |
||
return self._eadata._data | ||
|
||
@property | ||
jbrockmendel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def tz(self): | ||
# GH#18595 | ||
return self._eadata.tz | ||
|
||
@tz.setter | ||
def tz(self, value): | ||
# GH#3746; DatetimeArray will raise to disallow setting | ||
self._eadata.tz = value | ||
|
||
tzinfo = tz | ||
|
||
# Compat for frequency inference, see GH#23789 | ||
_is_monotonic_increasing = Index.is_monotonic_increasing | ||
|
@@ -1168,18 +1171,6 @@ def offset(self, value): | |
warnings.warn(msg, FutureWarning, stacklevel=2) | ||
self.freq = value | ||
|
||
@property | ||
def freq(self): | ||
return self._freq | ||
|
||
@freq.setter | ||
def freq(self, value): | ||
if value is not None: | ||
# let DatetimeArray to validation | ||
self._eadata.freq = value | ||
|
||
self._freq = to_offset(value) | ||
|
||
def __getitem__(self, key): | ||
result = self._eadata.__getitem__(key) | ||
if is_scalar(result): | ||
|
Uh oh!
There was an error while loading. Please reload this page.