ENH: Support multiple opening hours intervals for BusinessHour #26400

howsiwei · 2019-05-15T04:57:28Z

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

pep8speaks · 2019-05-15T05:21:28Z

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-05-25 08:22:02 UTC

codecov · 2019-05-15T05:21:28Z

Codecov Report

Merging #26400 into master will decrease coverage by 50.5%.
The diff coverage is 25.24%.

@@             Coverage Diff             @@
##           master   #26400       +/-   ##
===========================================
- Coverage   91.69%   41.18%   -50.51%     
===========================================
  Files         174      174               
  Lines       50743    50792       +49     
===========================================
- Hits        46527    20917    -25610     
- Misses       4216    29875    +25659

Flag	Coverage Δ
#multiple	`?`
#single	`41.18% <25.24%> (-0.15%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`45.64% <25.24%> (-51.05%)`	⬇️
pandas/io/formats/latex.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/plotting.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.37%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.16%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.1%)`	⬇️
pandas/core/tools/numeric.py	`10.44% <0%> (-89.56%)`	⬇️
... and 130 more

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 3b24fb6...394bf0e. Read the comment docs.

codecov · 2019-05-15T05:21:28Z

Codecov Report

Merging #26400 into master will increase coverage by <.01%.
The diff coverage is 98.98%.

@@            Coverage Diff             @@
##           master   #26400      +/-   ##
==========================================
+ Coverage   91.74%   91.75%   +<.01%     
==========================================
  Files         174      174              
  Lines       50763    50801      +38     
==========================================
+ Hits        46575    46610      +35     
- Misses       4188     4191       +3

Flag	Coverage Δ
#multiple	`90.26% <98.98%> (ø)`	⬆️
#single	`41.74% <33.33%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/offsets.py	`96.79% <98.98%> (+0.1%)`	⬆️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`97.02% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.7% <0%> (+0.1%)`	⬆️

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 44d5498...68fc283. Read the comment docs.

gfyoung · 2019-05-18T00:44:05Z

pandas/tests/tseries/offsets/test_offsets.py

+        with pytest.raises(ValueError):
+            BusinessHour(start=['09:00', '11:00'], end=['10:00'])
+        with pytest.raises(ValueError):
+            BusinessHour(start=['09:00', '11:00'], end=['12:00', '20:00'])


Can you update the pytest.raises calls (for this entire test function) to include a check for the error message? Just pass in match parameter that indicates the expected message.

gfyoung · 2019-05-18T00:45:46Z

pandas/tseries/offsets.py

+            for st in self.start:
+                if other.time() <= st:
+                    return datetime(other.year, other.month, other.day,
+                                    st.hour, st.minute)


Comment your code to explain what all of this logic is for.

Comments added

mroeschke · 2019-05-20T01:50:40Z

pandas/tseries/offsets.py

+        if _iterable_not_string(start):
+            start = np.asarray(start)
+            if len(start) == 0:
+                raise ValueError('number of starting time cannot be 0')


"Must include at least 1 start time"

mroeschke · 2019-05-20T01:50:54Z

pandas/tseries/offsets.py

+        if _iterable_not_string(end):
+            end = np.asarray(end)
+            if len(end) == 0:
+                raise ValueError('number of ending time cannot be 0')


"Must include at least 1 end time"

mroeschke · 2019-05-20T01:53:34Z

pandas/tseries/offsets.py

+            total_secs += self._get_business_hours_by_sec(
+                end[i], start[(i + 1) % num_openings])
+        if total_secs != 24 * 60 * 60:
+            raise ValueError('invalid starting and ending time(s)')


Can you make this error message a little more informative?

mroeschke · 2019-05-20T01:55:34Z

pandas/tseries/offsets.py


-    def _next_opening_time(self, other):
+    def _next_opening_time(self, other, dir=1):


Could you use a different variable besides dir? (It's a python keyword)

Changed dir to sign

mroeschke · 2019-05-20T01:58:46Z

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]))
+        raise ValueError("dt should be a starting time")


Can you add a test where this is hit?

You mean where ValueError is raised here? It would never happen if only public methods are called.

Well can this ValueError get floated up to the user? If so, we should have a test to catch it.

Only if user calls _get_closing_time directly him/herself

@mroeschke so should this be tested? If yes, do we just run _get_closing_time directly in the test function?

So if this error can never be hit by the user using public methods, I'm still not sure if we entirely need this error.

Should I remove the raise statement? Or add a comment to indicate that it would never be hit?

If if st.hour == dt.hour and st.minute == dt.minute: is always guaranteed to get hit, then you can remove the error.

I have removed the error and added a comment.

howsiwei · 2019-06-03T08:23:10Z

For some reason this PR is automatically closed after rebasing on latest master. I have created a new PR at #26628 and have edited as per all the comments here.

howsiwei force-pushed the master branch 2 times, most recently from 7bfa8a6 to 12894ec Compare May 15, 2019 06:35

gfyoung added the Enhancement label May 18, 2019

gfyoung reviewed May 18, 2019

View reviewed changes

gfyoung requested a review from jreback May 18, 2019 00:47

mroeschke reviewed May 20, 2019

View reviewed changes

howsiwei closed this Jun 3, 2019

howsiwei force-pushed the master branch from 68fc283 to 6354580 Compare June 3, 2019 07:51

howsiwei mentioned this pull request Jun 3, 2019

ENH: Support multiple opening hours intervals for BusinessHour #26628

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Support multiple opening hours intervals for BusinessHour #26400

ENH: Support multiple opening hours intervals for BusinessHour #26400

howsiwei commented May 15, 2019

pep8speaks commented May 15, 2019 •

edited

Loading

codecov bot commented May 15, 2019

codecov bot commented May 15, 2019 •

edited

Loading

gfyoung May 18, 2019

howsiwei May 20, 2019

gfyoung May 18, 2019

howsiwei May 20, 2019

mroeschke May 20, 2019

howsiwei May 20, 2019

mroeschke May 20, 2019

howsiwei May 20, 2019

mroeschke May 20, 2019

howsiwei May 20, 2019

mroeschke May 20, 2019

howsiwei May 20, 2019 •

edited

Loading

mroeschke May 20, 2019

howsiwei May 20, 2019 •

edited

Loading

mroeschke May 20, 2019

howsiwei May 21, 2019 •

edited

Loading

howsiwei May 23, 2019

mroeschke May 23, 2019

howsiwei May 24, 2019

mroeschke May 24, 2019

howsiwei May 25, 2019

howsiwei commented Jun 3, 2019


		def _next_opening_time(self, other):
		def _next_opening_time(self, other, dir=1):

ENH: Support multiple opening hours intervals for BusinessHour #26400

ENH: Support multiple opening hours intervals for BusinessHour #26400

Conversation

howsiwei commented May 15, 2019

pep8speaks commented May 15, 2019 • edited Loading

Comment last updated at 2019-05-25 08:22:02 UTC

codecov bot commented May 15, 2019

Codecov Report

codecov bot commented May 15, 2019 • edited 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

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

howsiwei May 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howsiwei May 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howsiwei May 21, 2019 • edited Loading

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

howsiwei commented Jun 3, 2019

pep8speaks commented May 15, 2019 •

edited

Loading

codecov bot commented May 15, 2019 •

edited

Loading

howsiwei May 20, 2019 •

edited

Loading

howsiwei May 20, 2019 •

edited

Loading

howsiwei May 21, 2019 •

edited

Loading