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

CLN: CacheableOffset not Properly Used #4609

Closed
cancan101 opened this issue Aug 19, 2013 · 4 comments · Fixed by #4614
Closed

CLN: CacheableOffset not Properly Used #4609

cancan101 opened this issue Aug 19, 2013 · 4 comments · Fixed by #4614
Labels
Refactor Internal refactoring of code
Milestone

Comments

@cancan101
Copy link
Contributor

I believe that many of the DateOffsets that extend CacheableOffset do so incorrectly.

Looking at pandas.tseries.offsets, there are some subclasses of DateOffset are declared as class Foo(DateOffset, CacheableOffset) and some as class Foo(CacheableOffset, DateOffset).

All that CacheableOffset does is to set the class field _cacheable = True. DateOffset sets _cacheable = False. Therefore, depending on the order of the parent classes, in some cases _cacheable will be False and some cases True

@jreback
Copy link
Contributor

jreback commented Aug 19, 2013

ok...can you find an instance where this is 'wrong'? and a PR to clean up? (could just be some old code)

@cancan101
Copy link
Contributor Author

That code does not look old. I am not exactly sure how _cacheable is being used; I will have to look more closely. I can certainly provide a python example of how it is broken:

class Cache(object):
   _foo = True

class DT(object):
   _foo = False

class A(Cache, DT):
   pass

class Bob(DT, Cache):
   pass

B()._foo
False

A()._foo
True

@cancan101
Copy link
Contributor Author

On closer inspection, I do not see where either _cacheable or _should_cache is used.

@cancan101
Copy link
Contributor Author

I take that back. I found the following in tseries.index:

            if (offset._should_cache() and
                not (offset._normalize_cache and not _normalized) and
                    _naive_in_cache_range(start, end)):
                index = cls._cached_range(start, end, periods=periods,
                                          offset=offset, name=name)
            else:
                index = _generate_regular_range(start, end, periods, offset)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants