Fix offset inits, apply_index dtypes by jbrockmendel · Pull Request #19142 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation18 Commits17 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This PR does two main things:

Change the default kwargs for offset __init__ methods so that the defaults are valid; ATM WeekOfMonth() and LastWeekOfMonth() will both raise ValueError.
ATM Week.apply_index adds an int64 array to a DatetimeIndex. This happens to have the desired output because the int64 array represents nanoseconds. This explicitly casts the array to timedelta64[ns] to make this explicit. This is a necessary step towards fixing DatetimeIndex + ndarray[int] wrong, reverse op errors #19123.

In addition there are two small pieces of refactoring:

Remove liboffsets.EndMixin since it is only mixed in to Week.
Use liboffsets.roll_convention to de-duplicate some code in liboffsets.shift_months.

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…days add timedelta64[ns] array instead of int64 array

Travis looks like a moto problem.

@@ -1226,7 +1225,8 @@ def _get_roll(self, i, before_day_of_month, after_day_of_month):
return roll

def _apply_index_days(self, i, roll):
i += (roll % 2) * Timedelta(days=self.day_of_month).value
nanos = (roll % 2) * Timedelta(days=self.day_of_month).value

Choose a reason for hiding this comment

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

I would rather

n = Timedelta((roll % 2) * Timedelta(days=self.day_of_month).value)
return i + n + Timedelta(days-1)

Choose a reason for hiding this comment

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

roll is an array

Choose a reason for hiding this comment

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

then pls add a doc-string (same below)

@@ -1271,13 +1271,14 @@ def _get_roll(self, i, before_day_of_month, after_day_of_month):
return roll

def _apply_index_days(self, i, roll):
return i + (roll % 2) * Timedelta(days=self.day_of_month - 1).value
nanos = (roll % 2) * Timedelta(days=self.day_of_month - 1).value

Choose a reason for hiding this comment

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

same

@@ -1394,7 +1411,7 @@ class WeekOfMonth(_WeekOfMonthMixin, DateOffset):
_prefix = 'WOM'
_adjust_dst = True

def __init__(self, n=1, normalize=False, week=None, weekday=None):

Choose a reason for hiding this comment

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

this doesn't break anything???

Choose a reason for hiding this comment

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

The default kwargs raise ATM.

Choose a reason for hiding this comment

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

where is the test for this? why isn't it raising currently?

@@ -1226,7 +1225,8 @@ def _get_roll(self, i, before_day_of_month, after_day_of_month):
return roll

def _apply_index_days(self, i, roll):
i += (roll % 2) * Timedelta(days=self.day_of_month).value
nanos = (roll % 2) * Timedelta(days=self.day_of_month).value

Choose a reason for hiding this comment

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

then pls add a doc-string (same below)

@@ -1394,7 +1411,7 @@ class WeekOfMonth(_WeekOfMonthMixin, DateOffset):
_prefix = 'WOM'
_adjust_dst = True

def __init__(self, n=1, normalize=False, week=None, weekday=None):

Choose a reason for hiding this comment

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

where is the test for this? why isn't it raising currently?

return self._end_apply_index(i)

def _end_apply_index(self, dtindex):
"""Offsets index to end of Period frequency"""

Choose a reason for hiding this comment

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

doc-string

Choose a reason for hiding this comment

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

Will do.

where is the test for this? why isn't it raising currently?

Until a few dozen PRs ago all that was passed to __init__ was n=1, normalize=False, **kwargs. The tests had to explicitly pass the relevant kwargs or else you'd get a KeyError. So there aren't any tests that use the defaults.

Until a few dozen PRs ago all that was passed to init was n=1, normalize=False, **kwargs. The tests had to explicitly pass the relevant kwargs or else you'd get a KeyError. So there aren't any tests that use the defaults.

ok can you add some pls.

since pd.offsets.WeekOfMonth() now works, pls add a note in bug fixes for whatsnew. otherwise lgtm. ping on green.

@@ -474,3 +474,4 @@ Other

- Improved error message when attempting to use a Python keyword as an identifier in a ``numexpr`` backed query (:issue:`18221`)
- :func:`Timestamp.replace` will now handle Daylight Savings transitions gracefully (:issue:`18319`)
- Bug in :class:`WeekOfMonth` and :class:`LastWeekOfMonth` where default keyword arguments for constructor raised ``ValueError`` (:issue:`19142`)

Choose a reason for hiding this comment

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

pls move this and the one above to COnversion, don't put things Other unless they obviously don't fit elsewhere. You could make a TimesSeries section if you want (and move appropriate things), but move this for now.

Choose a reason for hiding this comment

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

OK. For future reference, what heuristic should I be using that maps WeekOfMonth.__init__ ValueError to "conversion"?

2 participants