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

DOC: DateOffset.is_anchored track down intention, fix docstring #55388

Closed
jbrockmendel opened this issue Oct 3, 2023 · 8 comments · Fixed by #56594
Closed

DOC: DateOffset.is_anchored track down intention, fix docstring #55388

jbrockmendel opened this issue Oct 3, 2023 · 8 comments · Fixed by #56594
Assignees
Labels
Clean Index Related to the Index class or subclasses Internals Related to non-user accessible pandas implementation

Comments

@jbrockmendel
Copy link
Member

jbrockmendel commented Oct 3, 2023

def is_anchored(self) -> bool:
        # TODO: Does this make sense for the general case?  It would help
        # if there were a canonical docstring for what is_anchored means.

The docstring on the base class is_anchored pretty clearly doesn't make sense in the non-base class methods, so I suspect it is wrong. It has never been clear to me exactly what the intention of is_anchored is, but my best guess is something like offsets that are "anchored" on a particular day of the week or month of the year or etc.

The task here is to dig into the GH history to determine if the intent and correct behavior was ever clear. If so, to fix the implementation and/or docstring (and then remove the comment). The method isn't used outside the tests and I doubt many users use it, so OK to deprecate if it isn't useful or doesn't make sense.

I suspect something like it can make sense and be useful for DatetimeIndex set operations to determine when we can use RangeIndex fastpaths, xref #44025.

For historical digging/grepping it may be useful to know that this used to be isAnchored.

@jbrockmendel jbrockmendel added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Oct 3, 2023
@tpackard1
Copy link
Contributor

I'll start looking into the GH history and based on what I have found, then make a PR if it is warranted or come back here with my findings if intent and correct is ambiguous.

@tpackard1
Copy link
Contributor

take

@lithomas1 lithomas1 added Internals Related to non-user accessible pandas implementation Clean Index Related to the Index class or subclasses and removed Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Oct 4, 2023
@tpackard1
Copy link
Contributor

Hi @jbrockmendel, I have been looking through the pandas historical releases code and have gone back as far as v0.18.0 where isAnchored was still in use and the code for `DateOffset.isAnchored is as follows:

    def isAnchored(self):
        return (self.n == 1)

The above provides even less detail information about what the use case would be for the class DateOffset. And @jbrockmendel your comment that, "The method isn't used outside the tests and I doubt many users use it, ...", seems to be ringing true as I cannot determine a use case for this method, so I think depreciation may be the route to take but I will keep going back in GH history if you think it is necessary? Or I will go ahead and submit a PR to depreciate it if that is the correct course.

@jbrockmendel
Copy link
Member Author

Thanks for taking a look @tpackard1. I'm going to defer to @natmokval on this.

@natmokval
Copy link
Contributor

@jbrockmendel, sorry for taking a while to get into this.

I dug into the GH history and I found the first appearance of isAnchored

commit c6b236db73ff81007909be6406f0e484edc4a9eb (HEAD)
Author: Wes McKinney <wesmckinn@gmail.com>
Date:   Wed Aug 5 03:30:16 2009 +0000

We define isAnchored in pandas/core/datetools.py

class Week(DateOffset):
    def isAnchored(self):
        return (self.n == 1 and self.dayOfWeek is not None)

class BQuarterEnd(DateOffset):
   def isAnchored(self):
        return (self.n == 1 and self.startingMonth is not None)

and for classes DateOffset / BDay / BMonthEnd the definition is the same:

def isAnchored(self):
    return (self.n == 1)

isAnchored is called only once in constructor of the class DateRange in pandas/core/daterange.py (link)
In case we have anchored offset we call classmethod getCachedRange

if offset.isAnchored() and not isinstance(offset, datetools.Tick):
    index = cls.getCachedRange(fromDate, toDate, periods=periods,
                                           offset=offset)

@natmokval
Copy link
Contributor

natmokval commented Nov 26, 2023

The first time isAnchored appeared in 2009, see commit c6b236db73ff81007909be6406f0e484edc4a9eb
Definitions of isAnchored were quite similar to ones we use now. The idea behind: offsets with n=1 are used more often than offsets with n > 1, so we didn’t want to compute offsets with n=1 every time. To reuse them we used cache.
We called isAnchore only in constructor of the class DateRange in order to save this offset to cachedRange

Then in 2012, see commit 0cc1a3d0bc4617440e3df9e3e55b19cb6fb51a2d all definitions isAnchored were moved to offsets.py and we used isAnchored in _should_cache, which we called in constructor of class DatetimeIndex.

Then in 2019, see commit 1d36851ffdc8391b8b60a7628ce5a536180ef13b we deprecated isAnchored and started to use is_anchored instead (there was no connection between is_anchored and cache since then). We used is_anchored in def first and in def intersection

@natmokval
Copy link
Contributor

natmokval commented Nov 26, 2023

Seems like now we don't use is_anchored as it was supposed to be used at the beginning.
I checked: is_anchored is called only in tests, several test_is_anchored, and one test_constructor_timestamp.

I think we can either deprecate is_anchored or correct documentation for is_anchored. I suggest to deprecate it. What do you think @jbrockmendel?

@jbrockmendel
Copy link
Member Author

Agreed let's deprecate this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Index Related to the Index class or subclasses Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants