Skip to content
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

DEPR: the method is_anchored() for offsets #56594

Merged
merged 7 commits into from Jan 10, 2024

Conversation

natmokval
Copy link
Contributor

@natmokval natmokval commented Dec 21, 2023

deprecated the method is_anchored() for offsets classes: Tick, Week, QuarterOffset, FY5253Mixin, BaseOffset

@natmokval natmokval changed the title DEPR: is_anchored() DEPR: the method is_anchored() for offsets Dec 28, 2023
@natmokval natmokval marked this pull request as ready for review December 28, 2023 15:46
@natmokval
Copy link
Contributor Author

natmokval commented Dec 28, 2023

in this PR is_anchored() is deprecated for offsets. @MarcoGorelli, could you please take a look at my PR?

@MarcoGorelli
Copy link
Member

nice! @jbrockmendel I think you'd suggested this, fancy taking a look?

@@ -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`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no parentheses

Copy link
Contributor Author

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

@@ -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`)
Copy link
Member

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)

Copy link
Contributor Author

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

"""
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.
Copy link
Member

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's done

"""
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()``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Use obj.n == 1"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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.",
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


Examples
--------
>>> pd.offsets.Tick().is_anchored()
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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

@natmokval
Copy link
Contributor Author

I found some inconsistency in the behavior of the classes BusinessHour and CustomBusinessHour.
We always return False for pd.offsets.Hour().is_anchored()
But unexpectedly I get True for

pd.offsets.BusinessHour().is_anchored()
pd.offsets.CustomBusinessHour().is_anchored()

I am not sure, is it a bug?

And messages in the FutureWarning are different:

Hour.is_anchored is deprecated and will be removed in a future version, please use False instead.
BusinessHour.is_anchored is deprecated and will be removed in a future version, please use 'obj.n == 1' instead.

The reason is they are non-Tick subclasses.

Should we fix it before we deprecate is_anchored? What do you think, @jbrockmendel?

@MarcoGorelli
Copy link
Member

just for reference, here's the (only) use-case I found of is_anchored in the wild:

https://github.com/pastas/pastas/blob/dbe3275e012143605e3a162bfbb8ff0983d966e6/pastas/timeseries_utils.py#L160-L185

It wouldn't work for converting '2W-Tue' to '14D' though, so I'm not sure is_anchored is what they really wanted in the first place

I doubt anyone's using it for business hour / custom business hour - I think it's fine to just deprecate

@MarcoGorelli
Copy link
Member

/preview

Copy link
Contributor

github-actions bot commented Jan 9, 2024

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.",
Copy link
Member

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?

Copy link
Contributor Author

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.

def __init__(
self, n=1, normalize=False, weekday=0, startingMonth=1, variation="nearest"

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Member

@MarcoGorelli MarcoGorelli left a 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

@MarcoGorelli MarcoGorelli added this to the 2.2 milestone Jan 10, 2024
@MarcoGorelli MarcoGorelli added Frequency DateOffsets Deprecate Functionality to remove in pandas labels Jan 10, 2024
@MarcoGorelli MarcoGorelli merged commit f40475f into pandas-dev:main Jan 10, 2024
56 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Jan 10, 2024
@natmokval
Copy link
Contributor Author

thanks @MarcoGorelli for reviewing this PR

MarcoGorelli pushed a commit that referenced this pull request Jan 10, 2024
…r offsets) (#56813)

Backport PR #56594: DEPR: the method is_anchored() for offsets

Co-authored-by: Natalia Mokeeva <91160475+natmokval@users.noreply.github.com>
False
"""
warnings.warn(
f"{type(self).__name__}.is_anchored is deprecated and will be removed "
Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

@phofl phofl Jan 12, 2024

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

Copy link
Member

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.

Copy link
Member

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

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Frequency DateOffsets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: DateOffset.is_anchored track down intention, fix docstring
4 participants