From 6100c3fc6dbd306951a27b1ab872d5445402b293 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Tue, 12 Dec 2017 21:48:41 -0800
Subject: [PATCH 01/12] implement roll_monthday, roll_qtrday, notes on possible
 bugs

---
 pandas/_libs/tslibs/offsets.pyx | 49 ++++++++++++++---
 pandas/tseries/offsets.py       | 93 ++++++++++++---------------------
 2 files changed, 75 insertions(+), 67 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index 29e14103dfe20..5884a768e9019 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -540,11 +540,9 @@ def shift_quarters(int64_t[:] dtindex, int quarters,
                 n = quarters
 
                 months_since = (dts.month - q1start_month) % modby
-                compare_month = dts.month - months_since
-                compare_month = compare_month or 12
                 # compare_day is only relevant for comparison in the case
                 # where months_since == 0.
-                compare_day = get_firstbday(dts.year, compare_month)
+                compare_day = get_firstbday(dts.year, dts.month)
 
                 if n <= 0 and (months_since != 0 or
                                (months_since == 0 and dts.day > compare_day)):
@@ -573,12 +571,10 @@ def shift_quarters(int64_t[:] dtindex, int quarters,
                 n = quarters
 
                 months_since = (dts.month - q1start_month) % modby
-                compare_month = dts.month - months_since
-                compare_month = compare_month or 12
                 # compare_day is only relevant for comparison in the case
                 # where months_since == 0.
-                compare_day = get_lastbday(dts.year, compare_month)
-
+                compare_day = get_lastbday(dts.year, dts.month)
+                #
                 if n <= 0 and (months_since != 0 or
                                (months_since == 0 and dts.day > compare_day)):
                     # make sure to roll forward, so negate
@@ -794,6 +790,7 @@ cpdef datetime shift_month(datetime stamp, int months, object day_opt=None):
     return stamp.replace(year=year, month=month, day=day)
 
 
+# TODO: Can we declare this so it will take datetime _or_ pandas_datetimestruct
 cpdef int get_day_of_month(datetime other, day_opt) except? -1:
     """
     Find the day in `other`'s month that satisfies a DateOffset's onOffset
@@ -844,6 +841,42 @@ cpdef int get_day_of_month(datetime other, day_opt) except? -1:
         raise ValueError(day_opt)
 
 
+def _roll_monthday(n, other, compare):
+    # Either `other` and `compare` are _both_ datetimes or they are both
+    # integers for days in the same month.
+
+    if n > 0 and other < compare:
+        n -= 1
+    elif n <= 0 and other > compare:
+        # as if rolled forward already
+        n += 1
+    return n
+
+
+cpdef inline int roll_qtrday(other, n, month, day_opt='start',
+                             int modby=3) except? -1:
+    # TODO: type `other` as datetime-or-pandas_datetimestruct?
+    # TODO: Merge this with roll_yearday by setting modby=12 there?
+    #       code de-duplication versus perf hit?
+    # TODO: with small adjustments this could be used in shift_quarters
+    months_since = other.month % modby - month % modby
+
+    if n > 0:
+        if months_since < 0 or (months_since == 0 and
+                                other.day < get_day_of_month(other,
+                                                             day_opt)):
+            # pretend to roll back if on same month but
+            # before compare_day
+            n -= 1
+    else:
+        if (months_since > 0 or (months_since == 0 and
+                                  other.day > get_day_of_month(other,
+                                                               day_opt))):
+            # make sure to roll forward, so negate
+            n += 1
+    return n
+
+
 cpdef int roll_yearday(other, n, month, day_opt='start') except? -1:
     """
     Possibly increment or decrement the number of periods to shift
@@ -905,7 +938,7 @@ cpdef int roll_yearday(other, n, month, day_opt='start') except? -1:
                                    other.day < get_day_of_month(other,
                                                                 day_opt)):
             n -= 1
-    elif n <= 0:
+    else:
         if other.month > month or (other.month == month and
                                    other.day > get_day_of_month(other,
                                                                 day_opt)):
diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index dd5f01a36a43e..f56dae32ca29b 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -944,15 +944,8 @@ def onOffset(self, dt):
 
     @apply_wraps
     def apply(self, other):
-        n = self.n
         compare_day = self._get_offset_day(other)
-
-        if n > 0 and other.day < compare_day:
-            n -= 1
-        elif n <= 0 and other.day > compare_day:
-            # as if rolled forward already
-            n += 1
-
+        n = liboffsets._roll_monthday(self.n, other.day, compare_day)
         return shift_month(other, n, self._day_opt)
 
     @apply_index_wraps
@@ -1038,22 +1031,13 @@ class CustomBusinessMonthEnd(_CustomBusinessMonth):
 
     @apply_wraps
     def apply(self, other):
-        n = self.n
-
         # First move to month offset
         cur_mend = self.m_offset.rollforward(other)
 
         # Find this custom month offset
-        cur_cmend = self.cbday.rollback(cur_mend)
+        compare_day = self.cbday.rollback(cur_mend)
 
-        # handle zero case. arbitrarily rollforward
-        if n == 0 and other != cur_cmend:
-            n += 1
-
-        if other < cur_cmend and n >= 1:
-            n -= 1
-        elif other > cur_cmend and n <= -1:
-            n += 1
+        n = liboffsets._roll_monthday(self.n, other, compare_day)
 
         new = cur_mend + n * self.m_offset
         result = self.cbday.rollback(new)
@@ -1066,23 +1050,13 @@ class CustomBusinessMonthBegin(_CustomBusinessMonth):
 
     @apply_wraps
     def apply(self, other):
-        n = self.n
-        dt_in = other
-
         # First move to month offset
-        cur_mbegin = self.m_offset.rollback(dt_in)
+        cur_mbegin = self.m_offset.rollback(other)
 
         # Find this custom month offset
-        cur_cmbegin = self.cbday.rollforward(cur_mbegin)
+        compare_day = self.cbday.rollforward(cur_mbegin)
 
-        # handle zero case. arbitrarily rollforward
-        if n == 0 and dt_in != cur_cmbegin:
-            n += 1
-
-        if dt_in > cur_cmbegin and n <= -1:
-            n += 1
-        elif dt_in < cur_cmbegin and n >= 1:
-            n -= 1
+        n = liboffsets._roll_monthday(self.n, other, compare_day)
 
         new = cur_mbegin + n * self.m_offset
         result = self.cbday.rollforward(new)
@@ -1138,6 +1112,11 @@ def apply(self, other):
                 elif n == 0:
                     n = 1
 
+            # TODO: The remaining cases are for the first and last of the
+            # month; should we just shift there anyway and do other.replace
+            # irregardless?
+            # TODO: other.replace may not be DST safe.
+
         return self._apply(n, other)
 
     def _apply(self, n, other):
@@ -1259,6 +1238,8 @@ def onOffset(self, dt):
     def _apply(self, n, other):
         # if other.day is not day_of_month move to day_of_month and update n
         if other.day < self.day_of_month:
+            # TODO: Why does this not require n > 0 while
+            # SemiMonthEnd._apply does?
             n -= 1
         elif n <= 0 and other.day > self.day_of_month:
             n += 1
@@ -1343,6 +1324,7 @@ def onOffset(self, dt):
         if self.normalize and not _is_normalized(dt):
             return False
         return dt.weekday() == self.weekday
+        # TODO: Shouldn't this return True when self.weekday is None?
 
     @property
     def rule_code(self):
@@ -1404,15 +1386,12 @@ def apply(self, other):
         base = other
         offsetOfMonth = self.getOffsetOfMonth(other)
 
-        months = self.n
-        if months > 0 and offsetOfMonth > other:
-            months -= 1
-        elif months <= 0 and offsetOfMonth < other:
-            months += 1
+        months = liboffsets._roll_monthday(self.n, other, offsetOfMonth)
 
         other = self.getOffsetOfMonth(shift_month(other, months, 'start'))
         other = datetime(other.year, other.month, other.day, base.hour,
                          base.minute, base.second, base.microsecond)
+        # TODO: Why is this handled differently from LastWeekOfMonth?
         return other
 
     def getOffsetOfMonth(self, dt):
@@ -1485,11 +1464,7 @@ def __init__(self, n=1, normalize=False, weekday=None):
     def apply(self, other):
         offsetOfMonth = self.getOffsetOfMonth(other)
 
-        months = self.n
-        if months > 0 and offsetOfMonth > other:
-            months -= 1
-        elif months <= 0 and offsetOfMonth < other:
-            months += 1
+        months = liboffsets._roll_monthday(self.n, other, offsetOfMonth)
 
         return self.getOffsetOfMonth(shift_month(other, months, 'start'))
 
@@ -1497,6 +1472,8 @@ def getOffsetOfMonth(self, dt):
         m = MonthEnd()
         d = datetime(dt.year, dt.month, 1, dt.hour, dt.minute,
                      dt.second, dt.microsecond, tzinfo=dt.tzinfo)
+        # TODO: Will potentially dropping nanoseconds here be a problem?
+        # Particularly in onOffset?
         eom = m.rollforward(d)
         # TODO: Is this DST-safe?
         w = Week(weekday=self.weekday)
@@ -1532,7 +1509,8 @@ class QuarterOffset(DateOffset):
     _from_name_startingMonth = None
     _adjust_dst = True
     # TODO: Consider combining QuarterOffset and YearOffset __init__ at some
-    #       point
+    #       point.  Also apply_index, onOffset, rule_code if
+    #       startingMonth vs month attr names are resolved
 
     def __init__(self, n=1, normalize=False, startingMonth=None):
         self.n = self._validate_n(n)
@@ -1563,26 +1541,22 @@ def rule_code(self):
 
     @apply_wraps
     def apply(self, other):
-        n = self.n
-        compare_day = self._get_offset_day(other)
-
-        months_since = (other.month - self.startingMonth) % 3
-
-        if n <= 0 and (months_since != 0 or
-                       (months_since == 0 and other.day > compare_day)):
-            # make sure to roll forward, so negate
-            n += 1
-        elif n > 0 and (months_since == 0 and other.day < compare_day):
-            # pretend to roll back if on same month but before compare_day
-            n -= 1
-
-        return shift_month(other, 3 * n - months_since, self._day_opt)
+        # months_since: find the calendar quarter containing other.month,
+        # e.g. if other.month == 8, the calendar quarter is [Jul, Aug, Sep].
+        # Then find the month in that quarter containing an onOffset date for
+        # self.  `months_since` is the number of months to shift other.month
+        # to get to this on-offset month.
+        months_since = other.month % 3 - self.startingMonth % 3
+        qtrs = liboffsets.roll_qtrday(other, self.n, self.startingMonth,
+                                      day_opt=self._day_opt, modby=3)
+        months = qtrs * 3 - months_since
+        return shift_month(other, months, self._day_opt)
 
     def onOffset(self, dt):
         if self.normalize and not _is_normalized(dt):
             return False
-        modMonth = (dt.month - self.startingMonth) % 3
-        return modMonth == 0 and dt.day == self._get_offset_day(dt)
+        mod_month = (dt.month - self.startingMonth) % 3
+        return mod_month == 0 and dt.day == self._get_offset_day(dt)
 
     @apply_index_wraps
     def apply_index(self, dtindex):
@@ -2126,6 +2100,7 @@ def apply(self, other):
             n -= 1
         elif n < 0 and other > current_easter:
             n += 1
+        # TODO: Why does this handle the 0 case the opposite of others?
 
         # NOTE: easter returns a datetime.date so we have to convert to type of
         # other

From aac0832222280a8dba990132690c83b776dfce04 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Tue, 12 Dec 2017 22:38:37 -0800
Subject: [PATCH 02/12] simplify SemiMonth.apply

---
 pandas/tseries/offsets.py | 49 +++++++++++----------------------------
 1 file changed, 14 insertions(+), 35 deletions(-)

diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index f56dae32ca29b..040d89cce402a 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -1036,7 +1036,6 @@ def apply(self, other):
 
         # Find this custom month offset
         compare_day = self.cbday.rollback(cur_mend)
-
         n = liboffsets._roll_monthday(self.n, other, compare_day)
 
         new = cur_mend + n * self.m_offset
@@ -1055,7 +1054,6 @@ def apply(self, other):
 
         # Find this custom month offset
         compare_day = self.cbday.rollforward(cur_mbegin)
-
         n = liboffsets._roll_monthday(self.n, other, compare_day)
 
         new = cur_mbegin + n * self.m_offset
@@ -1096,26 +1094,21 @@ def rule_code(self):
 
     @apply_wraps
     def apply(self, other):
-        n = self.n
-        if not self.onOffset(other):
-            _, days_in_month = tslib.monthrange(other.year, other.month)
-            if 1 < other.day < self.day_of_month:
-                other = other.replace(day=self.day_of_month)
-                if n > 0:
-                    # rollforward so subtract 1
-                    n -= 1
-            elif self.day_of_month < other.day < days_in_month:
-                other = other.replace(day=self.day_of_month)
-                if n < 0:
-                    # rollforward in the negative direction so add 1
-                    n += 1
-                elif n == 0:
-                    n = 1
+        # shift `other` to self.day_of_month, incrementing `n` if necessary
+        n = liboffsets._roll_monthday(self.n, other.day, self.day_of_month)
+
+        days_in_month = tslib.monthrange(other.year, other.month)[1]
 
-            # TODO: The remaining cases are for the first and last of the
-            # month; should we just shift there anyway and do other.replace
-            # irregardless?
-            # TODO: other.replace may not be DST safe.
+        # For SemiMonthBegin on other.day == 1 and
+        # SemiMonthEnd on other.day == days_in_month,
+        # shifting `other` to `self.day_of_month` _always_ requires
+        # incrementing/decrementing `n`, regardless of whether it is
+        # initially positive.
+        if type(self) is SemiMonthBegin and (self.n <= 0 and other.day == 1):
+            n -= 1
+        elif type(self) is SemiMonthEnd and (self.n > 0 and
+                                             other.day == days_in_month):
+            n += 1
 
         return self._apply(n, other)
 
@@ -1185,12 +1178,6 @@ def onOffset(self, dt):
         return dt.day in (self.day_of_month, days_in_month)
 
     def _apply(self, n, other):
-        # if other.day is not day_of_month move to day_of_month and update n
-        if n > 0 and other.day < self.day_of_month:
-            n -= 1
-        elif other.day > self.day_of_month:
-            n += 1
-
         months = n // 2
         day = 31 if n % 2 else self.day_of_month
         return shift_month(other, months, day)
@@ -1236,14 +1223,6 @@ def onOffset(self, dt):
         return dt.day in (1, self.day_of_month)
 
     def _apply(self, n, other):
-        # if other.day is not day_of_month move to day_of_month and update n
-        if other.day < self.day_of_month:
-            # TODO: Why does this not require n > 0 while
-            # SemiMonthEnd._apply does?
-            n -= 1
-        elif n <= 0 and other.day > self.day_of_month:
-            n += 1
-
         months = n // 2 + n % 2
         day = 1 if n % 2 else self.day_of_month
         return shift_month(other, months, day)

From c5bc5b2a3ddec590a552794654fa7ada146fae2e Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Tue, 12 Dec 2017 22:42:56 -0800
Subject: [PATCH 03/12] whitespace/comment fixup

---
 pandas/_libs/tslibs/offsets.pyx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index 5884a768e9019..0263db01b61ef 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -574,7 +574,7 @@ def shift_quarters(int64_t[:] dtindex, int quarters,
                 # compare_day is only relevant for comparison in the case
                 # where months_since == 0.
                 compare_day = get_lastbday(dts.year, dts.month)
-                #
+
                 if n <= 0 and (months_since != 0 or
                                (months_since == 0 and dts.day > compare_day)):
                     # make sure to roll forward, so negate

From cb07e88eca1e5c57df4bd109db343c206c0ab7aa Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Wed, 13 Dec 2017 10:26:37 -0800
Subject: [PATCH 04/12] flake8 indentation cleanup

---
 pandas/_libs/tslibs/offsets.pyx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index a1a8bdf167e41..2514baab388ee 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -853,8 +853,8 @@ cpdef inline int roll_qtrday(other, n, month, day_opt='start',
             n -= 1
     else:
         if (months_since > 0 or (months_since == 0 and
-                                  other.day > get_day_of_month(other,
-                                                               day_opt))):
+                                 other.day > get_day_of_month(other,
+                                                              day_opt))):
             # make sure to roll forward, so negate
             n += 1
     return n

From eb5e72f58a8c3f2f71d7a0bf4bc4a2fcac54a335 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Wed, 13 Dec 2017 11:01:27 -0800
Subject: [PATCH 05/12] deprivatize and type roll_monthday

---
 pandas/_libs/tslibs/offsets.pyx |  2 +-
 pandas/tseries/offsets.py       | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index 2514baab388ee..2bbdc3e7d9b66 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -824,7 +824,7 @@ cpdef int get_day_of_month(datetime other, day_opt) except? -1:
         raise ValueError(day_opt)
 
 
-def _roll_monthday(n, other, compare):
+cpdef int roll_monthday(int n, other, compare):
     # Either `other` and `compare` are _both_ datetimes or they are both
     # integers for days in the same month.
 
diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index 9ce012a523e27..7334201e97495 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -945,7 +945,7 @@ def onOffset(self, dt):
     @apply_wraps
     def apply(self, other):
         compare_day = self._get_offset_day(other)
-        n = liboffsets._roll_monthday(self.n, other.day, compare_day)
+        n = liboffsets.roll_monthday(self.n, other.day, compare_day)
         return shift_month(other, n, self._day_opt)
 
     @apply_index_wraps
@@ -1036,7 +1036,7 @@ def apply(self, other):
 
         # Find this custom month offset
         compare_day = self.cbday.rollback(cur_mend)
-        n = liboffsets._roll_monthday(self.n, other, compare_day)
+        n = liboffsets.roll_monthday(self.n, other, compare_day)
 
         new = cur_mend + n * self.m_offset
         result = self.cbday.rollback(new)
@@ -1054,7 +1054,7 @@ def apply(self, other):
 
         # Find this custom month offset
         compare_day = self.cbday.rollforward(cur_mbegin)
-        n = liboffsets._roll_monthday(self.n, other, compare_day)
+        n = liboffsets.roll_monthday(self.n, other, compare_day)
 
         new = cur_mbegin + n * self.m_offset
         result = self.cbday.rollforward(new)
@@ -1095,7 +1095,7 @@ def rule_code(self):
     @apply_wraps
     def apply(self, other):
         # shift `other` to self.day_of_month, incrementing `n` if necessary
-        n = liboffsets._roll_monthday(self.n, other.day, self.day_of_month)
+        n = liboffsets.roll_monthday(self.n, other.day, self.day_of_month)
 
         days_in_month = tslib.monthrange(other.year, other.month)[1]
 
@@ -1366,7 +1366,7 @@ def apply(self, other):
         base = other
         offsetOfMonth = self.getOffsetOfMonth(other)
 
-        months = liboffsets._roll_monthday(self.n, other, offsetOfMonth)
+        months = liboffsets.roll_monthday(self.n, other, offsetOfMonth)
 
         other = self.getOffsetOfMonth(shift_month(other, months, 'start'))
         other = datetime(other.year, other.month, other.day, base.hour,
@@ -1444,7 +1444,7 @@ def __init__(self, n=1, normalize=False, weekday=None):
     def apply(self, other):
         offsetOfMonth = self.getOffsetOfMonth(other)
 
-        months = liboffsets._roll_monthday(self.n, other, offsetOfMonth)
+        months = liboffsets.roll_monthday(self.n, other, offsetOfMonth)
 
         return self.getOffsetOfMonth(shift_month(other, months, 'start'))
 

From ec3b24d13b9f05465dedbcf31752dbadd040da69 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Wed, 13 Dec 2017 13:23:42 -0800
Subject: [PATCH 06/12] docstrings, de-privatize, types

---
 pandas/_libs/tslibs/offsets.pyx | 40 ++++++++++++++++++++++++++++++---
 pandas/tseries/offsets.py       | 12 +++++-----
 2 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index 2bbdc3e7d9b66..54ff19e84765d 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -824,7 +824,23 @@ cpdef int get_day_of_month(datetime other, day_opt) except? -1:
         raise ValueError(day_opt)
 
 
-cpdef int roll_monthday(int n, other, compare):
+cpdef int roll_monthday(other, int n, compare):
+    """
+    Possibly increment or decrement the number of periods to shift
+    based on rollforward/rollbackward conventions.
+
+    Parameters
+    ----------
+    other : datetime or Timestamp
+    n : number of periods to increment, before adjusting for rolling
+    day_opt : 'start', 'end', 'business_start', 'business_end'
+        The convention to use in finding the day in a given month against
+        which to compare for rollforward/rollbackward decisions.
+
+    Returns
+    -------
+    n : int number of periods to increment
+    """
     # Either `other` and `compare` are _both_ datetimes or they are both
     # integers for days in the same month.
 
@@ -836,8 +852,25 @@ cpdef int roll_monthday(int n, other, compare):
     return n
 
 
-cpdef inline int roll_qtrday(other, n, month, day_opt='start',
-                             int modby=3) except? -1:
+cpdef int roll_qtrday(datetime other, int n, int month, day_opt='start',
+                      int modby=3) except? -1:
+    """
+    Possibly increment or decrement the number of periods to shift
+    based on rollforward/rollbackward conventions.
+
+    Parameters
+    ----------
+    other : datetime or Timestamp
+    n : number of periods to increment, before adjusting for rolling
+    month : reference month giving the first month of the year
+    day_opt : 'start', 'end', 'business_start', 'business_end'
+        The convention to use in finding the day in a given month against
+        which to compare for rollforward/rollbackward decisions.
+
+    Returns
+    -------
+    n : int number of periods to increment
+    """
     # TODO: type `other` as datetime-or-pandas_datetimestruct?
     # TODO: Merge this with roll_yearday by setting modby=12 there?
     #       code de-duplication versus perf hit?
@@ -869,6 +902,7 @@ cpdef int roll_yearday(other, n, month, day_opt='start') except? -1:
     ----------
     other : datetime or Timestamp
     n : number of periods to increment, before adjusting for rolling
+    month : reference month giving the first month of the year
     day_opt : 'start', 'end'
         'start': returns 1
         'end': returns last day  of the month
diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index 7334201e97495..633e1dca37423 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -945,7 +945,7 @@ def onOffset(self, dt):
     @apply_wraps
     def apply(self, other):
         compare_day = self._get_offset_day(other)
-        n = liboffsets.roll_monthday(self.n, other.day, compare_day)
+        n = liboffsets.roll_monthday(other.day, self.n, compare_day)
         return shift_month(other, n, self._day_opt)
 
     @apply_index_wraps
@@ -1036,7 +1036,7 @@ def apply(self, other):
 
         # Find this custom month offset
         compare_day = self.cbday.rollback(cur_mend)
-        n = liboffsets.roll_monthday(self.n, other, compare_day)
+        n = liboffsets.roll_monthday(other, self.n, compare_day)
 
         new = cur_mend + n * self.m_offset
         result = self.cbday.rollback(new)
@@ -1054,7 +1054,7 @@ def apply(self, other):
 
         # Find this custom month offset
         compare_day = self.cbday.rollforward(cur_mbegin)
-        n = liboffsets.roll_monthday(self.n, other, compare_day)
+        n = liboffsets.roll_monthday(other, self.n, compare_day)
 
         new = cur_mbegin + n * self.m_offset
         result = self.cbday.rollforward(new)
@@ -1095,7 +1095,7 @@ def rule_code(self):
     @apply_wraps
     def apply(self, other):
         # shift `other` to self.day_of_month, incrementing `n` if necessary
-        n = liboffsets.roll_monthday(self.n, other.day, self.day_of_month)
+        n = liboffsets.roll_monthday(other.day, self.n, self.day_of_month)
 
         days_in_month = tslib.monthrange(other.year, other.month)[1]
 
@@ -1366,7 +1366,7 @@ def apply(self, other):
         base = other
         offsetOfMonth = self.getOffsetOfMonth(other)
 
-        months = liboffsets.roll_monthday(self.n, other, offsetOfMonth)
+        months = liboffsets.roll_monthday(other, self.n, offsetOfMonth)
 
         other = self.getOffsetOfMonth(shift_month(other, months, 'start'))
         other = datetime(other.year, other.month, other.day, base.hour,
@@ -1444,7 +1444,7 @@ def __init__(self, n=1, normalize=False, weekday=None):
     def apply(self, other):
         offsetOfMonth = self.getOffsetOfMonth(other)
 
-        months = liboffsets.roll_monthday(self.n, other, offsetOfMonth)
+        months = liboffsets.roll_monthday(other, self.n, offsetOfMonth)
 
         return self.getOffsetOfMonth(shift_month(other, months, 'start'))
 

From c6f025b88f48da304d9475357d00192cdc41e9c4 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Wed, 13 Dec 2017 15:24:03 -0800
Subject: [PATCH 07/12] avoid while loop[ in BusinessDay.apply

---
 pandas/tseries/offsets.py | 41 +++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index 633e1dca37423..3232846d162fa 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -532,28 +532,31 @@ def get_str(td):
     def apply(self, other):
         if isinstance(other, datetime):
             n = self.n
+            wday = other.weekday()
 
-            if n == 0 and other.weekday() > 4:
-                n = 1
-
-            result = other
-
-            # avoid slowness below
-            if abs(n) > 5:
-                k = n // 5
-                result = result + timedelta(7 * k)
-                if n < 0 and result.weekday() > 4:
-                    n += 1
-                n -= 5 * k
-                if n == 0 and result.weekday() > 4:
-                    n -= 1
+            # avoid slowness below by operating on weeks first
+            weeks = n // 5
+            if n <= 0 and wday > 4:
+                # roll forward
+                n += 1
 
-            while n != 0:
-                k = n // abs(n)
-                result = result + timedelta(k)
-                if result.weekday() < 5:
-                    n -= k
+            n -= 5 * weeks
+
+            # n is always >= 0 at this point
+            if n == 0 and wday > 4:
+                # roll back
+                days = 4 - wday
+            elif wday > 4:
+                # roll forward
+                days = (7 - wday) + (n - 1)
+            elif wday + n <= 4:
+                # shift by n days without leaving the current week
+                days = n
+            else:
+                # shift by n days plus 2 to get past the weekend
+                days = n + 2
 
+            result = other + timedelta(days=7 * weeks + days)
             if self.offset:
                 result = result + self.offset
             return result

From f5694ded6571a82fff0cc3aa72a40a03137d5056 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Thu, 14 Dec 2017 08:09:03 -0800
Subject: [PATCH 08/12] docstring edits

---
 pandas/_libs/tslibs/offsets.pyx | 11 +++++------
 pandas/tseries/offsets.py       |  2 --
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index 54ff19e84765d..f7f05ec7e6f5d 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -831,11 +831,9 @@ cpdef int roll_monthday(other, int n, compare):
 
     Parameters
     ----------
-    other : datetime or Timestamp
+    other : datetime or int
     n : number of periods to increment, before adjusting for rolling
-    day_opt : 'start', 'end', 'business_start', 'business_end'
-        The convention to use in finding the day in a given month against
-        which to compare for rollforward/rollbackward decisions.
+    compare : datetime or int (must match `other`)
 
     Returns
     -------
@@ -862,10 +860,11 @@ cpdef int roll_qtrday(datetime other, int n, int month, day_opt='start',
     ----------
     other : datetime or Timestamp
     n : number of periods to increment, before adjusting for rolling
-    month : reference month giving the first month of the year
+    month : int reference month giving the first month of the year
     day_opt : 'start', 'end', 'business_start', 'business_end'
         The convention to use in finding the day in a given month against
         which to compare for rollforward/rollbackward decisions.
+    modby : int 3 for quarters, 12 for years
 
     Returns
     -------
@@ -913,7 +912,7 @@ cpdef int roll_yearday(other, n, month, day_opt='start') except? -1:
 
     Notes
     -----
-    * Mirrors `roll_check` in tslib.shift_months
+    * Mirrors `roll_check` in shift_months
 
     Examples
     -------
diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index 3232846d162fa..68d53b3f9ee2d 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -1446,9 +1446,7 @@ def __init__(self, n=1, normalize=False, weekday=None):
     @apply_wraps
     def apply(self, other):
         offsetOfMonth = self.getOffsetOfMonth(other)
-
         months = liboffsets.roll_monthday(other, self.n, offsetOfMonth)
-
         return self.getOffsetOfMonth(shift_month(other, months, 'start'))
 
     def getOffsetOfMonth(self, dt):

From 2566bf55e3a487858cf05ecb217b7f042f94610e Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Thu, 14 Dec 2017 08:10:38 -0800
Subject: [PATCH 09/12] add typing

---
 pandas/_libs/tslibs/offsets.pyx | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index f7f05ec7e6f5d..6f5fdeb72de96 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -892,7 +892,8 @@ cpdef int roll_qtrday(datetime other, int n, int month, day_opt='start',
     return n
 
 
-cpdef int roll_yearday(other, n, month, day_opt='start') except? -1:
+cpdef int roll_yearday(datetime other, int n, int month,
+                       object day_opt='start') except? -1:
     """
     Possibly increment or decrement the number of periods to shift
     based on rollforward/rollbackward conventions.

From a64923899e87c76b4ed6b0d21ee51c5c26a41e41 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Thu, 14 Dec 2017 15:29:13 -0800
Subject: [PATCH 10/12] dummy commit to force CI

---
 pandas/tseries/offsets.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index 68d53b3f9ee2d..b7bd97bfdcbee 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -22,7 +22,7 @@
 from pandas._libs.tslibs.offsets import (
     ApplyTypeError,
     as_datetime, _is_normalized,
-    _get_calendar, _to_dt64, _validate_business_time,
+    _get_calendar, _to_dt64,
     _determine_offset,
     apply_index_wraps,
     roll_yearday,
@@ -592,8 +592,8 @@ class BusinessHourMixin(BusinessMixin):
     def __init__(self, start='09:00', end='17:00', offset=timedelta(0)):
         # must be validated here to equality check
         kwds = {'offset': offset}
-        self.start = kwds['start'] = _validate_business_time(start)
-        self.end = kwds['end'] = _validate_business_time(end)
+        self.start = kwds['start'] = liboffsets._validate_business_time(start)
+        self.end = kwds['end'] = liboffsets._validate_business_time(end)
         self.kwds.update(kwds)
         self._offset = offset
 

From 71f138d3b68c49900604a19a2b31d5c71abd96d0 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Fri, 15 Dec 2017 17:36:40 -0800
Subject: [PATCH 11/12] Separate int and datetime roll funcs

---
 pandas/_libs/tslibs/offsets.pyx | 33 ++++++++++++++++++++++++++-------
 pandas/tseries/offsets.py       | 12 ++++++------
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index 6f5fdeb72de96..ff2a9e3e8604e 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -773,7 +773,6 @@ cpdef datetime shift_month(datetime stamp, int months, object day_opt=None):
     return stamp.replace(year=year, month=month, day=day)
 
 
-# TODO: Can we declare this so it will take datetime _or_ pandas_datetimestruct
 cpdef int get_day_of_month(datetime other, day_opt) except? -1:
     """
     Find the day in `other`'s month that satisfies a DateOffset's onOffset
@@ -824,24 +823,45 @@ cpdef int get_day_of_month(datetime other, day_opt) except? -1:
         raise ValueError(day_opt)
 
 
-cpdef int roll_monthday(other, int n, compare):
+cpdef int _roll_convention(int other, int n, int compare):
     """
     Possibly increment or decrement the number of periods to shift
     based on rollforward/rollbackward conventions.
 
     Parameters
     ----------
-    other : datetime or int
+    other : int, generally the day component of a datetime
     n : number of periods to increment, before adjusting for rolling
-    compare : datetime or int (must match `other`)
+    compare : int, generally the day component of a datetime, in the same
+              month as the datetime form which `other` was taken.
 
     Returns
     -------
     n : int number of periods to increment
     """
-    # Either `other` and `compare` are _both_ datetimes or they are both
-    # integers for days in the same month.
+    if n > 0 and other < compare:
+        n -= 1
+    elif n <= 0 and other > compare:
+        # as if rolled forward already
+        n += 1
+    return n
+
 
+cpdef int roll_monthday(datetime other, int n, datetime compare):
+    """
+    Possibly increment or decrement the number of periods to shift
+    based on rollforward/rollbackward conventions.
+
+    Parameters
+    ----------
+    other : datetime
+    n : number of periods to increment, before adjusting for rolling
+    compare : datetime
+
+    Returns
+    -------
+    n : int number of periods to increment
+    """
     if n > 0 and other < compare:
         n -= 1
     elif n <= 0 and other > compare:
@@ -870,7 +890,6 @@ cpdef int roll_qtrday(datetime other, int n, int month, day_opt='start',
     -------
     n : int number of periods to increment
     """
-    # TODO: type `other` as datetime-or-pandas_datetimestruct?
     # TODO: Merge this with roll_yearday by setting modby=12 there?
     #       code de-duplication versus perf hit?
     # TODO: with small adjustments this could be used in shift_quarters
diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index b7bd97bfdcbee..e4aa728e5b124 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -948,7 +948,7 @@ def onOffset(self, dt):
     @apply_wraps
     def apply(self, other):
         compare_day = self._get_offset_day(other)
-        n = liboffsets.roll_monthday(other.day, self.n, compare_day)
+        n = liboffsets._roll_convention(other.day, self.n, compare_day)
         return shift_month(other, n, self._day_opt)
 
     @apply_index_wraps
@@ -1038,8 +1038,8 @@ def apply(self, other):
         cur_mend = self.m_offset.rollforward(other)
 
         # Find this custom month offset
-        compare_day = self.cbday.rollback(cur_mend)
-        n = liboffsets.roll_monthday(other, self.n, compare_day)
+        compare_date = self.cbday.rollback(cur_mend)
+        n = liboffsets.roll_monthday(other, self.n, compare_date)
 
         new = cur_mend + n * self.m_offset
         result = self.cbday.rollback(new)
@@ -1056,8 +1056,8 @@ def apply(self, other):
         cur_mbegin = self.m_offset.rollback(other)
 
         # Find this custom month offset
-        compare_day = self.cbday.rollforward(cur_mbegin)
-        n = liboffsets.roll_monthday(other, self.n, compare_day)
+        compare_date = self.cbday.rollforward(cur_mbegin)
+        n = liboffsets.roll_monthday(other, self.n, compare_date)
 
         new = cur_mbegin + n * self.m_offset
         result = self.cbday.rollforward(new)
@@ -1098,7 +1098,7 @@ def rule_code(self):
     @apply_wraps
     def apply(self, other):
         # shift `other` to self.day_of_month, incrementing `n` if necessary
-        n = liboffsets.roll_monthday(other.day, self.n, self.day_of_month)
+        n = liboffsets._roll_convention(other.day, self.n, self.day_of_month)
 
         days_in_month = tslib.monthrange(other.year, other.month)[1]
 

From 9993a9132dd861c13161526402dc97f283e78341 Mon Sep 17 00:00:00 2001
From: Brock Mendel <jbrockmendel@gmail.com>
Date: Fri, 29 Dec 2017 10:19:54 -0800
Subject: [PATCH 12/12] add tests for liboffsets funcs

---
 pandas/_libs/tslibs/offsets.pyx               | 12 +--
 .../tests/tseries/offsets/test_liboffsets.py  | 91 +++++++++++++++++++
 pandas/tseries/offsets.py                     |  4 +-
 3 files changed, 99 insertions(+), 8 deletions(-)

diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index ff2a9e3e8604e..d3278e42e413f 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -823,7 +823,7 @@ cpdef int get_day_of_month(datetime other, day_opt) except? -1:
         raise ValueError(day_opt)
 
 
-cpdef int _roll_convention(int other, int n, int compare):
+cpdef int roll_convention(int other, int n, int compare):
     """
     Possibly increment or decrement the number of periods to shift
     based on rollforward/rollbackward conventions.
@@ -870,7 +870,7 @@ cpdef int roll_monthday(datetime other, int n, datetime compare):
     return n
 
 
-cpdef int roll_qtrday(datetime other, int n, int month, day_opt='start',
+cpdef int roll_qtrday(datetime other, int n, int month, object day_opt,
                       int modby=3) except? -1:
     """
     Possibly increment or decrement the number of periods to shift
@@ -903,16 +903,16 @@ cpdef int roll_qtrday(datetime other, int n, int month, day_opt='start',
             # before compare_day
             n -= 1
     else:
-        if (months_since > 0 or (months_since == 0 and
-                                 other.day > get_day_of_month(other,
-                                                              day_opt))):
+        if months_since > 0 or (months_since == 0 and
+                                other.day > get_day_of_month(other,
+                                                             day_opt)):
             # make sure to roll forward, so negate
             n += 1
     return n
 
 
 cpdef int roll_yearday(datetime other, int n, int month,
-                       object day_opt='start') except? -1:
+                       object day_opt) except? -1:
     """
     Possibly increment or decrement the number of periods to shift
     based on rollforward/rollbackward conventions.
diff --git a/pandas/tests/tseries/offsets/test_liboffsets.py b/pandas/tests/tseries/offsets/test_liboffsets.py
index 8aa32bc600ee6..1e0ecc39084eb 100644
--- a/pandas/tests/tseries/offsets/test_liboffsets.py
+++ b/pandas/tests/tseries/offsets/test_liboffsets.py
@@ -9,6 +9,7 @@
 from pandas import Timestamp
 
 import pandas._libs.tslibs.offsets as liboffsets
+from pandas._libs.tslibs.offsets import roll_qtrday
 
 
 def test_get_lastbday():
@@ -95,3 +96,93 @@ def test_roll_yearday():
     assert liboffsets.roll_yearday(other, 5, month, day_opt) == 5
     assert liboffsets.roll_yearday(other, -7, month, day_opt) == -6
     assert liboffsets.roll_yearday(other, 0, month, day_opt) == 1
+
+
+def test_roll_qtrday():
+    other = Timestamp(2072, 10, 1, 6, 17, 18)  # Saturday
+    for day_opt in ['start', 'end', 'business_start', 'business_end']:
+        # as long as (other.month % 3) != (month % 3), day_opt is irrelevant
+        # the `day_opt` doesn't matter.
+        month = 5  # (other.month % 3) < (month % 3)
+        assert roll_qtrday(other, 4, month, day_opt, modby=3) == 3
+        assert roll_qtrday(other, -3, month, day_opt, modby=3) == -3
+
+        month = 3  # (other.month % 3) > (month % 3)
+        assert roll_qtrday(other, 4, month, day_opt, modby=3) == 4
+        assert roll_qtrday(other, -3, month, day_opt, modby=3) == -2
+
+    month = 2
+    other = datetime(1999, 5, 31)  # Monday
+    # has (other.month % 3) == (month % 3)
+
+    n = 2
+    assert roll_qtrday(other, n, month, 'start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'end', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_end', modby=3) == n
+
+    n = -1
+    assert roll_qtrday(other, n, month, 'start', modby=3) == n + 1
+    assert roll_qtrday(other, n, month, 'end', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_start', modby=3) == n + 1
+    assert roll_qtrday(other, n, month, 'business_end', modby=3) == n
+
+    other = Timestamp(2072, 10, 1, 6, 17, 18)  # Saturday
+    month = 4  # (other.month % 3) == (month % 3)
+    n = 2
+    assert roll_qtrday(other, n, month, 'start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'end', modby=3) == n - 1
+    assert roll_qtrday(other, n, month, 'business_start', modby=3) == n - 1
+    assert roll_qtrday(other, n, month, 'business_end', modby=3) == n - 1
+
+    n = -1
+    assert roll_qtrday(other, n, month, 'start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'end', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_end', modby=3) == n
+
+    other = Timestamp(2072, 10, 3, 6, 17, 18)  # First businessday
+    month = 4  # (other.month % 3) == (month % 3)
+    n = 2
+    assert roll_qtrday(other, n, month, 'start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'end', modby=3) == n - 1
+    assert roll_qtrday(other, n, month, 'business_start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_end', modby=3) == n - 1
+
+    n = -1
+    assert roll_qtrday(other, n, month, 'start', modby=3) == n + 1
+    assert roll_qtrday(other, n, month, 'end', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_start', modby=3) == n
+    assert roll_qtrday(other, n, month, 'business_end', modby=3) == n
+
+
+def test_roll_monthday():
+    other = Timestamp('2017-12-29', tz='US/Pacific')
+    before = Timestamp('2017-12-01', tz='US/Pacific')
+    after = Timestamp('2017-12-31', tz='US/Pacific')
+
+    n = 42
+    assert liboffsets.roll_monthday(other, n, other) == n
+    assert liboffsets.roll_monthday(other, n, before) == n
+    assert liboffsets.roll_monthday(other, n, after) == n - 1
+
+    n = -4
+    assert liboffsets.roll_monthday(other, n, other) == n
+    assert liboffsets.roll_monthday(other, n, before) == n + 1
+    assert liboffsets.roll_monthday(other, n, after) == n
+
+
+def test_roll_convention():
+    other = 29
+    before = 1
+    after = 31
+
+    n = 42
+    assert liboffsets.roll_convention(other, n, other) == n
+    assert liboffsets.roll_convention(other, n, before) == n
+    assert liboffsets.roll_convention(other, n, after) == n - 1
+
+    n = -4
+    assert liboffsets.roll_convention(other, n, other) == n
+    assert liboffsets.roll_convention(other, n, before) == n + 1
+    assert liboffsets.roll_convention(other, n, after) == n
diff --git a/pandas/tseries/offsets.py b/pandas/tseries/offsets.py
index 5c412fdb439dd..008d9b5341148 100644
--- a/pandas/tseries/offsets.py
+++ b/pandas/tseries/offsets.py
@@ -973,7 +973,7 @@ def onOffset(self, dt):
     @apply_wraps
     def apply(self, other):
         compare_day = self._get_offset_day(other)
-        n = liboffsets._roll_convention(other.day, self.n, compare_day)
+        n = liboffsets.roll_convention(other.day, self.n, compare_day)
         return shift_month(other, n, self._day_opt)
 
     @apply_index_wraps
@@ -1123,7 +1123,7 @@ def rule_code(self):
     @apply_wraps
     def apply(self, other):
         # shift `other` to self.day_of_month, incrementing `n` if necessary
-        n = liboffsets._roll_convention(other.day, self.n, self.day_of_month)
+        n = liboffsets.roll_convention(other.day, self.n, self.day_of_month)
 
         days_in_month = tslib.monthrange(other.year, other.month)[1]