Skip to content

Bitesize offsets #17318

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Sep 23, 2017
Merged

Bitesize offsets #17318

Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3822f99
Fix typos
jbrockmendel Aug 23, 2017
309dd54
unify __repr__ and freqstr implementations
jbrockmendel Aug 23, 2017
87f659b
Remove methods identical to those of parent class
jbrockmendel Aug 23, 2017
3d1e6f8
alias ._offset-->.offset so freqstr compat
jbrockmendel Aug 23, 2017
0f5b2a6
flake8 fixup
jbrockmendel Aug 23, 2017
c66b842
dummy commit to force CI
jbrockmendel Aug 28, 2017
f016855
Dummy commit to force CI
jbrockmendel Aug 28, 2017
1fbe61d
Merge branch 'master' of https://github.com/pandas-dev/pandas into bi…
jbrockmendel Sep 14, 2017
030afde
Merge branch 'master' of https://github.com/pandas-dev/pandas into bi…
jbrockmendel Sep 15, 2017
2ca3bf2
Merge branch 'master' of https://github.com/pandas-dev/pandas into bi…
jbrockmendel Sep 20, 2017
1f4b5cd
Make offset a property to alias _offset for backward compat
jbrockmendel Sep 20, 2017
d1e9161
Rename offset attribute in unpickled instance of older version
jbrockmendel Sep 22, 2017
3d3c2a6
reviewer comments, pop key, dont assert
jbrockmendel Sep 23, 2017
24f6eee
Merge branch 'master' of https://github.com/pandas-dev/pandas into bi…
jbrockmendel Sep 23, 2017
c22ee24
Reduce periods for B freq to avoid OverflowError
jbrockmendel Sep 23, 2017
e52a791
docstring per reviewer request
jbrockmendel Sep 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 44 additions & 81 deletions pandas/tseries/offsets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,37 +322,42 @@ def _params(self):

def __repr__(self):
className = getattr(self, '_outputName', type(self).__name__)

if abs(self.n) != 1:
plural = 's'
else:
plural = ''

Copy link
Contributor

Choose a reason for hiding this comment

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

this whole thing should just be moved to a separate function, much more clear this way

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you're referring to as "this whole thing".

Copy link
Contributor

Choose a reason for hiding this comment

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

this formatting function (e.g. repr should just call it, passing parameters in).

n_str = ""
if self.n != 1:
n_str = "%s * " % self.n

out = '<%s' % n_str + className + plural + self._repr_attrs() + '>'
return out

# TODO: Combine this with BusinessMixin version by defining a whitelisted
# set of attributes on each object rather than the existing behavior of
# iterating over internal ``__dict__``
def _repr_attrs(self):
exclude = set(['n', 'inc', 'normalize'])
attrs = []
for attr in sorted(self.__dict__):
if ((attr == 'kwds' and len(self.kwds) == 0) or
attr.startswith('_')):
if attr.startswith('_'):
continue
elif attr == 'kwds':
elif attr == 'kwds': # TODO: get rid of this
kwds_new = {}
for key in self.kwds:
if not hasattr(self, key):
kwds_new[key] = self.kwds[key]
if len(kwds_new) > 0:
attrs.append('='.join((attr, repr(kwds_new))))
else:
if attr not in exclude:
attrs.append('='.join((attr, repr(getattr(self, attr)))))

plural = ''
if abs(self.n) != 1:
plural = 's'
attrs.append('kwds=%s' % (kwds_new))
elif attr not in exclude:
value = getattr(self, attr)
attrs.append('%s=%s' % (attr, value))

n_str = ''
if self.n != 1:
n_str = '{n} * '.format(n=self.n)

attrs_str = ''
out = ''
if attrs:
attrs_str = ': ' + ', '.join(attrs)

repr_content = ''.join([n_str, className, plural, attrs_str])
out = '<{content}>'.format(content=repr_content)
out += ': ' + ', '.join(attrs)
return out

@property
Expand Down Expand Up @@ -506,8 +511,18 @@ def freqstr(self):
else:
fstr = code

try:
if self._offset:
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Not all subclasses define the _offset attribute. That is something I intend to standardize, but this PR is explicitly intended to be limited in scope.

fstr += self._offset_str()
except AttributeError:
# TODO: standardize `_offset` vs `offset` naming convention
pass

return fstr

def _offset_str(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

try not to add new things which just clutter up

Copy link
Member Author

Choose a reason for hiding this comment

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

_offset_str is currently a method of BusinessDay, which has its own implementation of freqstr. Adding this dummy method lets us get rid of the duplicate freqstr method. Same idea with __repr__/_repr_attrs. Strictly less clutter.

return ''

@property
def nanos(self):
raise ValueError("{name} is a non-fixed frequency".format(name=self))
Expand All @@ -526,24 +541,6 @@ def _from_name(cls, suffix=None):
class BusinessMixin(object):
""" mixin to business types to provide related functions """

# TODO: Combine this with DateOffset by defining a whitelisted set of
# attributes on each object rather than the existing behavior of iterating
# over internal ``__dict__``
def __repr__(self):
className = getattr(self, '_outputName', self.__class__.__name__)

plural = ''
if abs(self.n) != 1:
plural = 's'

n_str = ''
if self.n != 1:
n_str = '{n} * '.format(n=self.n)

repr_content = ''.join([n_str, className, plural, self._repr_attrs()])
out = '<{content}>'.format(content=repr_content)
return out

def _repr_attrs(self):
if self.offset:
attrs = ['offset={offset!r}'.format(offset=self.offset)]
Expand Down Expand Up @@ -593,23 +590,7 @@ def __init__(self, n=1, normalize=False, **kwds):
self.normalize = normalize
self.kwds = kwds
self.offset = kwds.get('offset', timedelta(0))

@property
def freqstr(self):
try:
code = self.rule_code
except NotImplementedError:
return repr(self)

if self.n != 1:
fstr = '{n}{code}'.format(n=self.n, code=code)
else:
fstr = code

if self.offset:
fstr += self._offset_str()

return fstr
self._offset = self.offset # alias for backward compat

def _offset_str(self):
def get_str(td):
Expand Down Expand Up @@ -642,9 +623,6 @@ def get_str(td):
else:
return '+' + repr(self.offset)

def isAnchored(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

pls don't simply remove things. instead in a separate PR deprecate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

The method is still available. It's inherited verbatim from the parent class.

return (self.n == 1)

@apply_wraps
def apply(self, other):
if isinstance(other, datetime):
Expand Down Expand Up @@ -709,6 +687,7 @@ def __init__(self, **kwds):
kwds['end'] = self._validate_time(kwds.get('end', '17:00'))
self.kwds = kwds
self.offset = kwds.get('offset', timedelta(0))
self._offset = self.offset # alias for backward compat
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you just define .offset as a property to return ._offset?

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely tried this, don't remember off the top why it didn't work. Maybe when the caffeine kicks in.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's try that type of refactor again.

self.start = kwds.get('start', '09:00')
self.end = kwds.get('end', '17:00')

Expand Down Expand Up @@ -775,7 +754,7 @@ def _get_business_hours_by_sec(self):
Return business hours in a day by seconds.
"""
if self._get_daytime_flag():
# create dummy datetime to calcurate businesshours in a day
# create dummy datetime to calculate businesshours in a day
dtstart = datetime(2014, 4, 1, self.start.hour, self.start.minute)
until = datetime(2014, 4, 1, self.end.hour, self.end.minute)
return tslib.tot_seconds(until - dtstart)
Expand Down Expand Up @@ -810,7 +789,7 @@ def rollforward(self, dt):

@apply_wraps
def apply(self, other):
# calcurate here because offset is not immutable
# calculate here because offset is not immutable
daytime = self._get_daytime_flag()
businesshours = self._get_business_hours_by_sec()
bhdelta = timedelta(seconds=businesshours)
Expand Down Expand Up @@ -859,7 +838,7 @@ def apply(self, other):
if n >= 0:
bday_edge = self._prev_opening_time(other)
bday_edge = bday_edge + bhdelta
# calcurate remainder
# calculate remainder
bday_remain = result - bday_edge
result = self._next_opening_time(other)
result += bday_remain
Expand Down Expand Up @@ -897,7 +876,7 @@ def onOffset(self, dt):

def _onOffset(self, dt, businesshours):
"""
Slight speedups using calcurated values
Slight speedups using calculated values
"""
# if self.normalize and not _is_normalized(dt):
# return False
Expand Down Expand Up @@ -978,6 +957,7 @@ def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
self.normalize = normalize
self.kwds = kwds
self.offset = kwds.get('offset', timedelta(0))
self._offset = self.offset # alias for backward compat
calendar, holidays = self.get_calendar(weekmask=weekmask,
holidays=holidays,
calendar=calendar)
Expand Down Expand Up @@ -1378,9 +1358,6 @@ def _apply_index_days(self, i, roll):
class BusinessMonthEnd(MonthOffset):
"""DateOffset increments between business EOM dates"""

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

@apply_wraps
def apply(self, other):
n = self.n
Expand Down Expand Up @@ -1471,6 +1448,7 @@ def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
self.normalize = normalize
self.kwds = kwds
self.offset = kwds.get('offset', timedelta(0))
self._offset = self.offset # alias for backward compat
self.cbday = CustomBusinessDay(n=self.n, normalize=normalize,
weekmask=weekmask, holidays=holidays,
calendar=calendar, **kwds)
Expand Down Expand Up @@ -1531,6 +1509,7 @@ def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
self.normalize = normalize
self.kwds = kwds
self.offset = kwds.get('offset', timedelta(0))
self._offset = self.offset # alias for backward compat
self.cbday = CustomBusinessDay(n=self.n, normalize=normalize,
weekmask=weekmask, holidays=holidays,
calendar=calendar, **kwds)
Expand Down Expand Up @@ -1985,16 +1964,6 @@ class QuarterEnd(QuarterOffset):
_default_startingMonth = 3
_prefix = 'Q'

def __init__(self, n=1, normalize=False, **kwds):
self.n = n
self.normalize = normalize
self.startingMonth = kwds.get('startingMonth', 3)

self.kwds = kwds

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

@apply_wraps
def apply(self, other):
n = self.n
Expand Down Expand Up @@ -2030,9 +1999,6 @@ class QuarterBegin(QuarterOffset):
_from_name_startingMonth = 1
_prefix = 'QS'

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

@apply_wraps
def apply(self, other):
n = self.n
Expand Down Expand Up @@ -2660,9 +2626,6 @@ class Easter(DateOffset):
"""
_adjust_dst = True

def __init__(self, n=1, **kwds):
super(Easter, self).__init__(n, **kwds)

@apply_wraps
def apply(self, other):
currentEaster = easter(other.year)
Expand Down