ENH: Support multiple opening hours intervals for BusinessHour #26628

howsiwei · 2019-06-03T08:00:14Z

closes ENH: BusinessHour to support multiple opening and closing times per day #15481
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I'm not sure why, but after rebasing on latest master my previous PR #26400 is automatically closed. So I created a new PR here. I have edited as per all the comments there.

codecov · 2019-06-03T08:35:08Z

Codecov Report

Merging #26628 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #26628   +/-   ##
=======================================
  Coverage   91.88%   91.88%           
=======================================
  Files         174      174           
  Lines       50692    50729   +37     
=======================================
+ Hits        46576    46610   +34     
- Misses       4116     4119    +3

Flag	Coverage Δ
#multiple	`90.41% <100%> (ø)`	⬆️
#single	`41.83% <33.67%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`96.87% <100%> (+0.18%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`97% <0%> (-0.12%)`	⬇️

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 23b0788...170d08b. Read the comment docs.

codecov · 2019-06-03T08:35:10Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #26628   +/-   ##
=========================================
  Coverage          ?   91.72%           
=========================================
  Files             ?      178           
  Lines             ?    50808           
  Branches          ?        0           
=========================================
  Hits              ?    46605           
  Misses            ?     4203           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.32% <100%> (?)`
#single	`41.23% <32.63%> (?)`

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`96.85% <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 deffee6...6b72cea. Read the comment docs.

WillAyd · 2019-06-03T22:58:09Z

Can you reopen the original PR and push this back there? Would be preferable to maintain comment history

howsiwei · 2019-06-04T10:30:45Z

Unfortunately I don't have the permission to reopen PR.

WillAyd · 2019-06-04T11:14:51Z

Hmm OK. Doesn't look like I can reopen either. Not sure entirely what happened but I do see it was pushed to your master there - would be best practice going forward to not push to master as an FYI

howsiwei · 2019-06-05T04:22:42Z

@WillAyd thanks for the advice. Any comment on the code changes?

WillAyd · 2019-06-05T13:09:42Z

cc @gfyoung and @mroeschke who were already looking at original PR

pandas/tests/tseries/offsets/test_offsets.py

pandas/tseries/offsets.py

pep8speaks · 2019-06-07T04:50:17Z

Hello @howsiwei! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-28 12:36:49 UTC

pandas/tests/tseries/offsets/test_offsets.py

mroeschke

One small testing comment otherwise looks pretty good to me.

mroeschke · 2019-06-08T18:08:48Z

doc/source/whatsnew/v0.25.0.rst

@@ -82,7 +82,7 @@ Other Enhancements
 - :meth:`DataFrame.query` and :meth:`DataFrame.eval` now supports quoting column names with backticks to refer to names with spaces (:issue:`6508`)
 - :func:`merge_asof` now gives a more clear error message when merge keys are categoricals that are not equal (:issue:`26136`)
 - :meth:`pandas.core.window.Rolling` supports exponential (or Poisson) window type (:issue:`21303`)
-
+- :class: `pandas.offsets.BusinessHour` supports multiple opening hours intervals


No space between :class: and pandas.offsets.BusinessHour. Also can you include the issue number this PR closes?

jreback · 2019-06-08T21:34:22Z

pandas/tseries/offsets.py

@@ -579,9 +580,46 @@ class BusinessHourMixin(BusinessMixin):

    def __init__(self, start='09:00', end='17:00', offset=timedelta(0)):
        # must be validated here to equality check
-        start = liboffsets._validate_business_time(start)
+        if _iterable_not_string(start):


use is_list_like

jreback · 2019-06-08T21:34:46Z

pandas/tseries/offsets.py

+                raise ValueError('Must include at least 1 start time')
+        else:
+            start = np.array([start])
+        if _iterable_not_string(end):


same

blank line before the if

jreback · 2019-06-08T21:36:24Z

pandas/tseries/offsets.py

@@ -579,9 +580,46 @@ class BusinessHourMixin(BusinessMixin):

    def __init__(self, start='09:00', end='17:00', offset=timedelta(0)):
        # must be validated here to equality check
-        start = liboffsets._validate_business_time(start)
+        if _iterable_not_string(start):
+            start = np.asarray(start)


I would do this like

if not is_like_like(start): start = [start] if not len(start): raise....

the np.asarray can be done when you validate

pandas/tseries/offsets.py

jreback · 2019-06-08T21:38:24Z

pandas/tseries/offsets.py

-            return True
-        else:
-            return False
+    def _get_daytime_flag(self, start, end):


can you add a doc-string

I removed this function as it's only use once and I think its clearer to just use start < end.

pandas/tseries/offsets.py

jreback · 2019-06-08T21:42:22Z

pandas/tseries/offsets.py

        """
        Return business hours in a day by seconds.
        """
-        if self._get_daytime_flag:
+        if self._get_daytime_flag(start, end):
            # create dummy datetime to calculate businesshours in a day


do same as above, only define hour, minute in the clauses

pandas/tseries/offsets.py

jreback · 2019-06-08T21:43:01Z

pandas/tseries/offsets.py

-                    bday_remain = result - bday_edge
-                    result = self._next_opening_time(other)
-                    result += bday_remain
+                while rem != timedelta(0):


can you add some comments on what you are doing.

can you use more meaningful variable names

pandas/tseries/offsets.py

jreback · 2019-06-08T21:43:54Z

we need to split test_offsets.py as its getting huge (can be after this PR).

cc @pandas-dev/pandas-core

codecov-io · 2019-06-11T19:01:38Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #26628   +/-   ##
=========================================
  Coverage          ?   92.04%           
=========================================
  Files             ?      180           
  Lines             ?    50741           
  Branches          ?        0           
=========================================
  Hits              ?    46704           
  Misses            ?     4037           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.68% <98.92%> (?)`
#single	`41.88% <30.1%> (?)`

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`96.45% <98.92%> (ø)`

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 de22a48...71f82c3. Read the comment docs.

howsiwei · 2019-06-11T19:38:48Z

pandas/tseries/offsets.py

@@ -576,12 +577,50 @@ def onOffset(self, dt):


 class BusinessHourMixin(BusinessMixin):
+    start: List[dt_time]
+    end: List[dt_time]
+    n: int


This is to suppress the mypy errors:

"BusinessHourMixin" has no attribute "start" "BusinessHourMixin" has no attribute "end" "BusinessHourMixin" has no attribute "n"

However this syntax is not compatible with python 3.5...
Any advice?

There's a lot of typing stuff only compatible with 3.5.2 onwards. It's an issue we need to address but orthogonal to this PR - is this causing some sort of error here?

It's causing SyntaxError: invalid syntax in python 3.5.

@WillAyd any work around for this problem? Or should I just remove the type hintings?

@WillAyd any suggestions?

You need to use the “# type: ...” syntax. The way you have it now is unfortunately only available for parameters in function signatures in Python 3.5

Thanks for the suggestion. I have removed the function signatures and it's green now.

mroeschke

LGTM

jbrockmendel · 2019-06-27T22:01:46Z

pandas/tseries/offsets.py

+        if not len(end):
+            raise ValueError('Must include at least 1 end time')
+
+        vliboffsets = np.vectorize(liboffsets._validate_business_time)


this is equivalent to start = np.array([liboffsets._validate_business_time(x) for x in start])? Does the vectorize really make a difference?

Yes they are equivalent. Is there a preference to use either way?

yes, @jbrockmendel is more idiomatic here.

jbrockmendel · 2019-06-27T22:02:50Z

pandas/tseries/offsets.py

+
+        Parameters
+        ----------
+        other : datetime


is this specifically not Timestamp?

No, it may not be Timestamp.

jbrockmendel · 2019-06-27T22:12:20Z

pandas/tseries/offsets.py

+            if st.hour == dt.hour and st.minute == dt.minute:
+                return dt + timedelta(
+                    seconds=self._get_business_hours_by_sec(st, self.end[i]))
+        assert False


Presumably this shouldn't be reachable? ValueError?

Yes, this is not reachable. Previously I raise ValueError here but later removed it following @mroeschke's suggestion #26400 (comment). assert False was added later to prevent mypy error.

jbrockmendel · 2019-06-27T22:13:22Z

Minor comments, generally looks good.

jreback · 2019-06-28T12:14:39Z

@howsiwei small change and can you merge master; ping on green.

…sinesshour

jreback · 2019-06-28T14:10:28Z

thanks @howsiwei nice patch!

…s-dev#26628)

Si Wei How and others added 4 commits June 3, 2019 15:42

Fix BusinessHour docstring

ecfc92e

Support multiple starting and ending times in BusinessHour

63f0b52

Edit comments and tests

4cd7db1

Edit error messages

170d08b

howsiwei mentioned this pull request Jun 3, 2019

ENH: Support multiple opening hours intervals for BusinessHour #26400

Closed

4 tasks

gfyoung added Enhancement Datetime Datetime data dtype labels Jun 5, 2019

mroeschke reviewed Jun 5, 2019

View reviewed changes

pandas/tests/tseries/offsets/test_offsets.py Outdated Show resolved Hide resolved

mroeschke reviewed Jun 5, 2019

View reviewed changes

pandas/tests/tseries/offsets/test_offsets.py Outdated Show resolved Hide resolved

mroeschke reviewed Jun 5, 2019

View reviewed changes

pandas/tests/tseries/offsets/test_offsets.py Outdated Show resolved Hide resolved

mroeschke reviewed Jun 5, 2019

View reviewed changes

pandas/tseries/offsets.py Show resolved Hide resolved

howsiwei force-pushed the businesshour branch 2 times, most recently from e3a849e to ec062e0 Compare June 7, 2019 14:13

Update tests

e39a688

howsiwei force-pushed the businesshour branch from ec062e0 to e39a688 Compare June 7, 2019 15:20

mroeschke reviewed Jun 7, 2019

View reviewed changes

pandas/tests/tseries/offsets/test_offsets.py Show resolved Hide resolved

mroeschke reviewed Jun 7, 2019

View reviewed changes

Add test

097a0fa

mroeschke reviewed Jun 8, 2019

View reviewed changes

jreback requested changes Jun 8, 2019

View reviewed changes

Update code and comments

1478845

howsiwei force-pushed the businesshour branch from cd1db4f to 1478845 Compare June 11, 2019 09:13

How Si Wei added 2 commits June 11, 2019 17:14

Merge remote-tracking branch 'upstream/master' into businesshour

2669ee4

Fix lint errors

7575597

howsiwei force-pushed the businesshour branch from fb81223 to 7575597 Compare June 11, 2019 18:03

Fix mypy errors

6b72cea

howsiwei force-pushed the businesshour branch from 8427cbe to 6b72cea Compare June 11, 2019 19:01

howsiwei commented Jun 11, 2019

View reviewed changes

simonjayhawkins mentioned this pull request Jun 12, 2019

Split large test modules #26807

Closed

6 tasks

howsiwei added 2 commits June 26, 2019 23:57

Remove type signatures as they don't play well with python 3.5

90fa744

Merge branch 'master' into businesshour

13fb339

mroeschke approved these changes Jun 27, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Jun 27, 2019

jreback approved these changes Jun 27, 2019

View reviewed changes

jbrockmendel reviewed Jun 27, 2019

View reviewed changes

mroeschke mentioned this pull request Jun 27, 2019

TST: Split test_offsets.py #27085

Closed

jbrockmendel reviewed Jun 27, 2019

View reviewed changes

howsiwei added 2 commits June 28, 2019 20:33

Make minor changes

6179197

Merge branch 'master' of https://github.com/pandas-dev/pandas into bu…

71f82c3

…sinesshour

jreback merged commit 7137938 into pandas-dev:master Jun 28, 2019

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Jun 28, 2019

ENH: Support multiple opening hours intervals for BusinessHour (panda…

c43fa06

…s-dev#26628)

ENH: Support multiple opening hours intervals for BusinessHour #26628

ENH: Support multiple opening hours intervals for BusinessHour #26628

Conversation

howsiwei commented Jun 3, 2019 • edited Loading

codecov bot commented Jun 3, 2019

Codecov Report

codecov bot commented Jun 3, 2019 • edited Loading

Codecov Report

WillAyd commented Jun 3, 2019 • edited Loading

howsiwei commented Jun 4, 2019

WillAyd commented Jun 4, 2019

howsiwei commented Jun 5, 2019

WillAyd commented Jun 5, 2019

pep8speaks commented Jun 7, 2019 • edited Loading

Comment last updated at 2019-06-28 12:36:49 UTC

mroeschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 8, 2019

codecov-io commented Jun 11, 2019 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jun 27, 2019

jreback commented Jun 28, 2019

jreback commented Jun 28, 2019

howsiwei commented Jun 3, 2019 •

edited

Loading

codecov bot commented Jun 3, 2019 •

edited

Loading

WillAyd commented Jun 3, 2019 •

edited

Loading

pep8speaks commented Jun 7, 2019 •

edited

Loading

codecov-io commented Jun 11, 2019 •

edited by codecov bot

Loading

WillAyd Jun 26, 2019 •

edited

Loading