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

Fix offset __inits__, apply_index dtypes #19142

Merged
merged 17 commits into from Jan 12, 2018

Conversation

Projects
None yet
2 participants
@jbrockmendel
Member

jbrockmendel commented Jan 8, 2018

This PR does two main things:

  1. Change the default kwargs for offset __init__ methods so that the defaults are valid; ATM WeekOfMonth() and LastWeekOfMonth() will both raise ValueError.

  2. 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 #19123.

In addition there are two small pieces of refactoring:

  1. Remove liboffsets.EndMixin since it is only mixed in to Week.

  2. 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
@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Jan 9, 2018

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

This comment has been minimized.

@jreback

jreback Jan 9, 2018

Contributor

I would rather

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

This comment has been minimized.

@jbrockmendel

jbrockmendel Jan 9, 2018

Member

roll is an array

This comment has been minimized.

@jreback

jreback Jan 11, 2018

Contributor

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

This comment has been minimized.

@jreback

jreback Jan 9, 2018

Contributor

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):

This comment has been minimized.

@jreback

jreback Jan 9, 2018

Contributor

this doesn't break anything???

This comment has been minimized.

@jbrockmendel

jbrockmendel Jan 9, 2018

Member

The default kwargs raise ATM.

This comment has been minimized.

@jreback

jreback Jan 11, 2018

Contributor

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

@codecov

This comment has been minimized.

codecov bot commented Jan 10, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@27a5039). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #19142   +/-   ##
=========================================
  Coverage          ?   91.52%           
=========================================
  Files             ?      147           
  Lines             ?    48785           
  Branches          ?        0           
=========================================
  Hits              ?    44651           
  Misses            ?     4134           
  Partials          ?        0
Flag Coverage Δ
#multiple 89.89% <100%> (?)
#single 41.61% <26.31%> (?)
Impacted Files Coverage Δ
pandas/tseries/offsets.py 97.09% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27a5039...4ea4e1e. Read the comment docs.

@@ -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

This comment has been minimized.

@jreback

jreback Jan 11, 2018

Contributor

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):

This comment has been minimized.

@jreback

jreback Jan 11, 2018

Contributor

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"""

This comment has been minimized.

@jreback

jreback Jan 11, 2018

Contributor

doc-string

This comment has been minimized.

@jbrockmendel
@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Jan 11, 2018

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.

@jreback

This comment has been minimized.

Contributor

jreback commented Jan 11, 2018

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.

@jreback jreback added this to the 0.23.0 milestone Jan 11, 2018

@jreback

This comment has been minimized.

Contributor

jreback commented Jan 11, 2018

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

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Jan 11, 2018

ping

@@ -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`)

This comment has been minimized.

@jreback

jreback Jan 12, 2018

Contributor

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.

This comment has been minimized.

@jbrockmendel

jbrockmendel Jan 12, 2018

Member

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

@jreback jreback merged commit 8912efc into pandas-dev:master Jan 12, 2018

0 of 3 checks passed

ci/circleci Your tests are queued behind your running builds
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Jan 12, 2018

thanks!

@jbrockmendel jbrockmendel deleted the jbrockmendel:offsets-intarray branch Feb 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment