-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
BUG: Fix freq setter for DatetimeIndex/TimedeltaIndex and deprecate for PeriodIndex #20772
Changes from 1 commit
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 |
---|---|---|
|
@@ -454,15 +454,7 @@ def __new__(cls, data=None, | |
|
||
if verify_integrity and len(subarr) > 0: | ||
if freq is not None and not freq_infer: | ||
inferred = subarr.inferred_freq | ||
if inferred != freq.freqstr: | ||
on_freq = cls._generate(subarr[0], None, len(subarr), None, | ||
freq, tz=tz, ambiguous=ambiguous) | ||
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. passing 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. Correct, and I don't think it was actually needed in the old version either. At the point at which this is being done, 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, yep, indeed, that sounds correct (and tested it with a small example) |
||
if not np.array_equal(subarr.asi8, on_freq.asi8): | ||
raise ValueError('Inferred frequency {0} from passed ' | ||
'dates does not conform to passed ' | ||
'frequency {1}' | ||
.format(inferred, freq.freqstr)) | ||
cls._validate_frequency(subarr, freq, ambiguous=ambiguous) | ||
|
||
if freq_infer: | ||
inferred = subarr.inferred_freq | ||
|
@@ -836,7 +828,7 @@ def __setstate__(self, state): | |
np.ndarray.__setstate__(data, nd_state) | ||
|
||
self.name = own_state[0] | ||
self.freq = own_state[1] | ||
self._freq = own_state[1] | ||
self._tz = timezones.tz_standardize(own_state[2]) | ||
|
||
# provide numpy < 1.7 compat | ||
|
@@ -1726,16 +1718,6 @@ def slice_indexer(self, start=None, end=None, step=None, kind=None): | |
else: | ||
raise | ||
|
||
@property | ||
def freq(self): | ||
"""get/set the frequency of the Index""" | ||
return self._freq | ||
|
||
@freq.setter | ||
def freq(self, value): | ||
"""get/set the frequency of the Index""" | ||
self._freq = value | ||
|
||
@property | ||
def offset(self): | ||
"""get/set the frequency of the Index""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -219,7 +219,7 @@ class PeriodIndex(DatelikeOps, DatetimeIndexOpsMixin, Int64Index): | |
_is_numeric_dtype = False | ||
_infer_as_myclass = True | ||
|
||
freq = None | ||
_freq = None | ||
|
||
_engine_type = libindex.PeriodEngine | ||
|
||
|
@@ -367,7 +367,7 @@ def _from_ordinals(cls, values, name=None, freq=None, **kwargs): | |
result.name = name | ||
if freq is None: | ||
raise ValueError('freq is not specified and cannot be inferred') | ||
result.freq = Period._maybe_convert_freq(freq) | ||
result._freq = Period._maybe_convert_freq(freq) | ||
result._reset_identity() | ||
return result | ||
|
||
|
@@ -560,6 +560,19 @@ def is_full(self): | |
values = self.values | ||
return ((values[1:] - values[:-1]) < 2).all() | ||
|
||
@property | ||
def freq(self): | ||
"""Return the frequency object if it is set, otherwise None""" | ||
return self._freq | ||
|
||
@freq.setter | ||
def freq(self, value): | ||
msg = ('Setting PeriodIndex.freq has been deprecated and will be ' | ||
'removed in a future version; use PeriodIndex.asfreq instead. ' | ||
'The PeriodIndex.freq setter is not guaranteed to work.') | ||
warnings.warn(msg, FutureWarning, stacklevel=2) | ||
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. Not sure if we really need to deprecate setting this: it's user facing, but I think the only case it works is if you set to the existing frequency. Could potentially remove the setter entirely, causing an 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. Yes, IMO it was buggy, and as you say, only if it was the same frequency, it was working. But given that corner case, it maybe does not hurt to do the deprecation I would say. I think the custom AttributeError message is a good idea in the future. |
||
self._freq = value | ||
|
||
def asfreq(self, freq=None, how='E'): | ||
""" | ||
Convert the PeriodIndex to the specified frequency `freq`. | ||
|
@@ -1060,7 +1073,7 @@ def __setstate__(self, state): | |
np.ndarray.__setstate__(data, nd_state) | ||
|
||
# backcompat | ||
self.freq = Period._maybe_convert_freq(own_state[1]) | ||
self._freq = Period._maybe_convert_freq(own_state[1]) | ||
|
||
else: # pragma: no cover | ||
data = np.empty(state) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is generating the index needed to know if it a correct frequence? Checking the inferred frequency is not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, see it was also done like that below (still wondering though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's necessary, at least to a certain extent. There are cases where multiple frequencies are valid for a given set of dates, so you can have a valid frequency that's not the inferred frequency, e.g.
['2018-01-01', '2018-01-02', '2018-01-03']
could be calendar day or business day. Users can also define custom frequencies that would need to be validated but would not be the inferred frequency.There are cases where you don't need to generate the entire index to reject invalid frequencies, e.g. if the second generated value doesn't match. But I don't immediately see how to get around generating the entire index to determine if a frequency is valid. I suppose you could maybe optimize memory usage with huge indexes by doing some type of elementwise validation, only keeping the current generated element in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, I see, that's for sure a valid case.
For me, improving the performance of the validation is not very high priority.