Make DateOffset.kwds a property #19403
Conversation
| @property | ||
| def kwds(self): | ||
| # for backwards-compatibility | ||
| kwds = {name: getattr(self, name, None) for name in self._attributes |
There was a problem hiding this comment.
In particular DateOffset has a lot of _attributes that may not get set unless specifically passed.
| @@ -218,7 +218,7 @@ def test_offset_freqstr(self, offset_types): | |||
|
|
|||
| freqstr = offset.freqstr | |||
| if freqstr not in ('<Easter>', | |||
| "<DateOffset: kwds={'days': 1}>", | |||
| "<DateOffset: days=1>", | |||
There was a problem hiding this comment.
need a whatsnew note, explainig that the repr changed
There was a problem hiding this comment.
did the repr change break anything? can you add some tests for this (doesn't have to be comprehensive, but some basic ones is ok)
pandas/tseries/offsets.py
Outdated
|
|
||
| self._offset, self._use_relativedelta = _determine_offset(kwds) | ||
| for name in kwds: |
There was a problem hiding this comment.
you can do
self.__dict__.update(kwds)
pandas/tseries/offsets.py
Outdated
| @@ -284,7 +288,7 @@ def isAnchored(self): | |||
| return (self.n == 1) | |||
|
|
|||
| def _params(self): | |||
| all_paras = dict(list(vars(self).items()) + list(self.kwds.items())) | |||
| all_paras = vars(self) | |||
There was a problem hiding this comment.
use self.__dict__ (as we do below) rather than vars
pandas/tseries/offsets.py
Outdated
| if 'offset' in state: | ||
| # Older versions have offset attribute instead of _offset | ||
| if '_offset' in state: # pragma: no cover | ||
| raise ValueError('Unexpected key `_offset`') |
pandas/tseries/offsets.py
Outdated
| def __setstate__(self, state): | ||
| """Reconstruct an instance from a pickled state""" | ||
| if 'offset' in state: | ||
| # Older versions have offset attribute instead of _offset |
There was a problem hiding this comment.
can you put a version note here (so late we can fix this)
| offset = state['_offset'] | ||
| odict = offset.__dict__ | ||
| kwds = {key: odict[key] for key in odict if odict[key]} | ||
| state.update(kwds) |
There was a problem hiding this comment.
why not just state.update(odict)?
There was a problem hiding this comment.
Because we don't want to pull in all of the unused relativedelta attributes.
Codecov Report
@@ Coverage Diff @@
## master #19403 +/- ##
==========================================
- Coverage 91.67% 91.67% -0.01%
==========================================
Files 148 148
Lines 48553 48554 +1
==========================================
Hits 44513 44513
- Misses 4040 4041 +1
Continue to review full report at Codecov.
|
| @@ -218,7 +218,7 @@ def test_offset_freqstr(self, offset_types): | |||
|
|
|||
| freqstr = offset.freqstr | |||
| if freqstr not in ('<Easter>', | |||
| "<DateOffset: kwds={'days': 1}>", | |||
| "<DateOffset: days=1>", | |||
There was a problem hiding this comment.
did the repr change break anything? can you add some tests for this (doesn't have to be comprehensive, but some basic ones is ok)
Well it broke that one test. Did you have something else in mind? |
|
looks fine. can you rebase and ping on green (just to make sure) |
|
ping |
|
thanks! |
Returning to an older goal of making DateOffset immutable, this PR moves towards getting rid of
DateOffset.kwdsby making it a property instead of regular attribute. This uses the_get_attributes_dictpattern, albeit without actually using a_get_attributes_dictmethod.I expect this to entail a small perf penalty since lookups are slower, but that's small potatoes next to the speedups we'll get from caching once these are immutable.