-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
DEPR: the method is_anchored() for offsets #56594
Conversation
900c44c
to
8c870f6
Compare
in this PR |
nice! @jbrockmendel I think you'd suggested this, fancy taking a look? |
doc/source/whatsnew/v2.2.0.rst
Outdated
@@ -592,6 +593,7 @@ Other Deprecations | |||
- Deprecated :meth:`Series.ravel`, the underlying array is already 1D, so ravel is not necessary (:issue:`52511`) | |||
- Deprecated :meth:`Series.resample` and :meth:`DataFrame.resample` with a :class:`PeriodIndex` (and the 'convention' keyword), convert to :class:`DatetimeIndex` (with ``.to_timestamp()``) before resampling instead (:issue:`53481`) | |||
- Deprecated :meth:`Series.view`, use :meth:`Series.astype` instead to change the dtype (:issue:`20251`) | |||
- Deprecated :meth:`offsets.Tick().is_anchored()`, use ``False`` instead (:issue:`55388`) |
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.
no parentheses
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.
Thank you for reviewing this PR. I removed the parentheses
doc/source/whatsnew/v2.2.0.rst
Outdated
@@ -584,6 +584,7 @@ Other Deprecations | |||
- Changed :meth:`Timedelta.resolution_string` to return ``h``, ``min``, ``s``, ``ms``, ``us``, and ``ns`` instead of ``H``, ``T``, ``S``, ``L``, ``U``, and ``N``, for compatibility with respective deprecations in frequency aliases (:issue:`52536`) | |||
- Deprecated :attr:`offsets.Day.delta`, :attr:`offsets.Hour.delta`, :attr:`offsets.Minute.delta`, :attr:`offsets.Second.delta`, :attr:`offsets.Milli.delta`, :attr:`offsets.Micro.delta`, :attr:`offsets.Nano.delta`, use ``pd.Timedelta(obj)`` instead (:issue:`55498`) | |||
- Deprecated :func:`pandas.api.types.is_interval` and :func:`pandas.api.types.is_period`, use ``isinstance(obj, pd.Interval)`` and ``isinstance(obj, pd.Period)`` instead (:issue:`55264`) | |||
- Deprecated :func:`pd.DateOffset().is_anchored()`, use ``DateOffset().n == 1`` instead (:issue:`55388`) |
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.
:meth:DateOffset.is_anchored
, use ``obj.n == 1` for non-Tick subclasses (for Tick this was always False)
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.
thanks, I did as you suggested
pandas/_libs/tslibs/offsets.pyx
Outdated
""" | ||
Return boolean whether the frequency is a unit frequency (n=1). | ||
|
||
.. deprecated:: 2.2.0 | ||
The is_anchored is deprecated and will be removed in a future version. |
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.
"The is_anchored is deprecated" -> "is_anchored is deprecated"
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.
it's done
pandas/_libs/tslibs/offsets.pyx
Outdated
""" | ||
Return boolean whether the frequency is a unit frequency (n=1). | ||
|
||
.. deprecated:: 2.2.0 | ||
The is_anchored is deprecated and will be removed in a future version. | ||
Do ``DateOffset().n == 1`` instead of ``DateOffset().is_anchored()``. |
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.
"Use obj.n == 1"
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.
done
pandas/_libs/tslibs/offsets.pyx
Outdated
Examples | ||
-------- | ||
>>> pd.DateOffset().is_anchored() | ||
True | ||
>>> pd.DateOffset(2).is_anchored() | ||
False | ||
""" | ||
warnings.warn( | ||
f"{type(self).__name__}.is_anchored() is deprecated and will be removed " | ||
f"in a future version, please use {type(self).__name__}.n == 1 instead.", |
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.
{type(self).name}.n -> obj.n
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.
done
pandas/_libs/tslibs/offsets.pyx
Outdated
|
||
Examples | ||
-------- | ||
>>> pd.offsets.Tick().is_anchored() |
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.
Tick should never be initialized directly; use a subclass here
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.
thank you, I will use Hour
@@ -84,7 +84,7 @@ def test_constructor_timestamp(self, closed, name, freq, periods, tz): | |||
tm.assert_index_equal(result, expected) | |||
|
|||
# GH 20976: linspace behavior defined from start/end/periods | |||
if not breaks.freq.is_anchored() and tz is None: | |||
if not breaks.freq.n == 1 and tz is None: | |||
# matches expected only for non-anchored offsets and tz naive |
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.
comment here refers to anchoring. i suspect the comment (which i suspect i wrote) misunderstood what "is_anchored" really meant
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.
thanks, I removed the comment
I found some inconsistency in the behavior of the classes
I am not sure, is it a bug? And messages in the
The reason is they are non-Tick subclasses. Should we fix it before we deprecate is_anchored? What do you think, @jbrockmendel? |
just for reference, here's the (only) use-case I found of It wouldn't work for converting I doubt anyone's using it for business hour / custom business hour - I think it's fine to just deprecate |
/preview |
Website preview of this PR available at: https://pandas.pydata.org/preview/56594/ |
@@ -3597,6 +3641,12 @@ cdef class FY5253Mixin(SingleConstructorOffset): | |||
self.variation = state.pop("variation") | |||
|
|||
def is_anchored(self) -> bool: | |||
warnings.warn( | |||
f"{type(self).__name__}.is_anchored is deprecated and will be removed " | |||
f"in a future version, please use \'obj.n == 1\' instead.", |
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.
do we not want to say about self.startingMonth is not None and self.weekday is not None
in this one?
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 am not sure about this. I think, there is no need to check if parameters startingMonth
and weekday
are not None
. For subclasses of the class FY5253Mixin
we initialize these parameters in constructor and set them 0 and 1 accordingly.
pandas/pandas/_libs/tslibs/offsets.pyx
Lines 3579 to 3580 in 1381f70
def __init__( | |
self, n=1, normalize=False, weekday=0, startingMonth=1, variation="nearest" |
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.
right, thanks, they can't actually be None
, so the is_anchored
definition was too complex to begin with
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.
if we check:
>>> pd.offsets.FY5253().is_anchored()
FutureWarning: FY5253.is_anchored is deprecated and will be removed in a future version, please use 'obj.n == 1' instead.
True
>>> pd.offsets.FY5253(n=1, startingMonth=1, weekday=1).is_anchored()
True
On the other hand for the class Week we have
>>> pd.offsets.Week(1).is_anchored()
FutureWarning: Week.is_anchored is deprecated and will be removed in a future version, please use 'obj.n == 1 and obj.weekday is not None' instead.
False
>>> pd.offsets.Week(n=1, weekday=0).is_anchored()
True
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.
but also, trying to pass None
just raises
In [12]: FY5253(n=1, weekday=1, startingMonth=1).is_anchored()
<ipython-input-12-37644e23496a>:1: FutureWarning: FY5253.is_anchored is deprecated and will be removed in a future version, please use 'obj.n == 1' instead.
FY5253(n=1, weekday=1, startingMonth=1).is_anchored()
Out[12]: True
In [13]: FY5253(n=1, weekday=None, startingMonth=None).is_anchored()
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[13], line 1
----> 1 FY5253(n=1, weekday=None, startingMonth=None).is_anchored()
File offsets.pyx:3627, in pandas._libs.tslibs.offsets.FY5253Mixin.__init__()
TypeError: an integer is required
In [14]: FY5253(n=1, weekday=None, startingMonth=1).is_anchored()
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[14], line 1
----> 1 FY5253(n=1, weekday=None, startingMonth=1).is_anchored()
File offsets.pyx:3628, in pandas._libs.tslibs.offsets.FY5253Mixin.__init__()
TypeError: an integer is required
In [15]: FY5253(n=1, weekday=1, startingMonth=None).is_anchored()
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[15], line 1
----> 1 FY5253(n=1, weekday=1, startingMonth=None).is_anchored()
File offsets.pyx:3627, in pandas._libs.tslibs.offsets.FY5253Mixin.__init__()
TypeError: an integer is required
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.
nice one, thanks @natmokval , let's backport this
thanks @MarcoGorelli for reviewing this PR |
False | ||
""" | ||
warnings.warn( | ||
f"{type(self).__name__}.is_anchored is deprecated and will be removed " |
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 don't understand this deprecation, how can I check for the equivalent attribute here? Use False is not really helpful
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.
for this class it's always False
for others you typically need to check that obj.n==1
and obj.weekday is None
(if it has a weekday attribute) and obj.startingMonth is None
(if it has a startingMonth
attribute)
what are you using it for?
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.
We are generally looking for freqs that aren’t anchored
I thought a little bit about this and the deprecations isn’t great, it makes those checks significantly more complex than before for users
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 thought that we can advise users use False, because is_anchored
always returns False
for Tick
subclasses.
Do you think it would be better to use some expression that is always False instead of False, like pd.offsets.Hour().n == None
?
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 mostly don’t want to have different check for every group, that’s what makes this ugly
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.
That helps, thx
We have to make a decision about shifting divisions, this basically goes back how the offset behaves on different values, e.g. if it can shift different values to the same target (if it's anchored for example) then we have to make a different decision compared to if every value maps to a distinct target value
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.
e.g. if it can shift different values to the same target (if it's anchored for example)
An example might help here. I think what you're referring to is what past-me thought is_anchored meant (xref #44025), e.g "W-SUN" would be anchored, but "W" would not (it would act like "7D"). pd.DateOffset(day=1)
would, but pd.DateOffset(days=1)
would not.
NB: the "is equivalent to" i gave about isn't quite right.
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.
idx = pd.Index([pd.Timestamp("2020-01-01"), pd.Timestamp("2020-01-02")])
print(idx.shift(1, freq=pd.tseries.offsets.Week(1)))
print(idx.shift(1, freq=pd.tseries.offsets.Week(1, weekday=2)))
DatetimeIndex(['2020-01-08', '2020-01-09'], dtype='datetime64[ns]', freq=None)
DatetimeIndex(['2020-01-08', '2020-01-08'], dtype='datetime64[ns]', freq=None)
The first one just shifts 7 days, so everything is fine
The second one shifts to a specific weekday though, which is an issue for us, because it maps 2 different days to the same target
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.
That matches what I think a more-useful is_anchored would mean. My guess is we can come up with examples (e.g. the day=1 vs days=1 above) where checking is_anchored would be wrong for your use case.
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.
idx = pd.Index([pd.Timestamp("2020-01-01"), pd.Timestamp("2020-01-02")])
print(idx.shift(1, freq=pd.tseries.offsets.Week(2)))
print(idx.shift(1, freq=pd.tseries.offsets.Week(2, weekday=2)))
DatetimeIndex(['2020-01-15', '2020-01-16'], dtype='datetime64[ns]', freq=None)
DatetimeIndex(['2020-01-15', '2020-01-15'], dtype='datetime64[ns]', freq=None)
The first one just shifts 14 days, and the second one shifts to a specific weekday
Yet neither "is_anchored":
In [21]: pd.tseries.offsets.Week(2).is_anchored()
Out[21]: False
In [22]: pd.tseries.offsets.Week(2, weekday=2).is_anchored()
Out[22]: False
deprecated the method
is_anchored()
for offsets classes:Tick, Week, QuarterOffset, FY5253Mixin, BaseOffset