Skip to content

Disable class level caching in AbstractHolidayCalendar #9552

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

Closed
dhirschfeld opened this issue Feb 25, 2015 · 2 comments · Fixed by #9555
Closed

Disable class level caching in AbstractHolidayCalendar #9552

dhirschfeld opened this issue Feb 25, 2015 · 2 comments · Fixed by #9555
Labels
Bug Datetime Datetime data dtype
Milestone

Comments

@dhirschfeld
Copy link
Contributor

By caching at the class level it prevents instances from defining different rules - e.g.

import numpy as np
from pandas.tseries.holiday import AbstractHolidayCalendar, Holiday

class HolidayCalendar(AbstractHolidayCalendar):
    def __init__(self, rules):
        self.rules = list(np.atleast_1d(rules))

jan1 = HolidayCalendar(Holiday('jan1', year=2015, month=1, day=1))
jan2 = HolidayCalendar(Holiday('jan2', year=2015, month=1, day=2))


jan1.holidays()
Out[2]: 
<class 'pandas.tseries.index.DatetimeIndex'>
[2015-01-01]
Length: 1, Freq: None, Timezone: None

jan2.holidays()
Out[3]: 
<class 'pandas.tseries.index.DatetimeIndex'>
[2015-01-01]    <--------------------------- uses cached result from *other* instance!
Length: 1, Freq: None, Timezone: None
import numpy as np
from pandas.tseries.holiday import AbstractHolidayCalendar, Holiday

class HolidayCalendar(AbstractHolidayCalendar):
    def __init__(self, rules):
        self.rules = list(np.atleast_1d(rules))

jan1 = HolidayCalendar(Holiday('jan1', year=2015, month=1, day=1))
jan2 = HolidayCalendar(Holiday('jan2', year=2015, month=1, day=2))


jan2.holidays()
Out[2]: 
<class 'pandas.tseries.index.DatetimeIndex'>
[2015-01-02]
Length: 1, Freq: None, Timezone: None

jan1.holidays()
Out[3]: 
<class 'pandas.tseries.index.DatetimeIndex'>
[2015-01-02]    <--------------------------- uses cached result from *other* instance!
Length: 1, Freq: None, Timezone: None

https://github.com/pydata/pandas/blob/a72d95163b4d268012709255d7a52bbe5c1a7eb6/pandas/tseries/holiday.py#L351-L357

@rockg
Copy link
Contributor

rockg commented Feb 25, 2015

The expected use of this is that there is only one set of rules per holiday class. Namely there is a one-to-one mapping between holiday calendars and rules. Ideally, if you change the rules you would create a new class. However, this isn't exactly obvious from how things work and I don't see the harm in making the cache on the instance rather than the class (see #6719) for some design discussion.

dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Feb 26, 2015
@dhirschfeld
Copy link
Contributor Author

That seems like a strange design to me - if the rules are a class level attribute then there is no instance level data stored on the class and all the methods could be classmethods and the user would never have to actually instantiate the subclass to use it.

It also seems contrary to AbstractHolidayCalendar.__init__ which specifically allows overriding the rules at an instance-level.

dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Feb 26, 2015
dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Feb 26, 2015
dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Feb 26, 2015
@jreback jreback added Bug Datetime Datetime data dtype labels Apr 13, 2015
@jreback jreback added this to the 0.16.1 milestone Apr 13, 2015
dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Apr 13, 2015
@jreback jreback changed the title Disable class level caching in AbstractHolidayCalendar Disable class level caching in AbstractHolidayCalendar Apr 13, 2015
dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Apr 13, 2015
dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Apr 13, 2015
dhirschfeld pushed a commit to dhirschfeld/pandas that referenced this issue Apr 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Datetime Datetime data dtype
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants