Skip to content
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

BUG: error calculating BusinessHourMixin.apply for long business hour per day #26381

Merged
merged 6 commits into from May 17, 2019

Conversation

@jiangyue12392
Copy link
Contributor

commented May 14, 2019

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew: handle the case where adding of business hour to the timestamp results in an ending time landing in the next business time period. Example

Input:

bh = BusinessHour(n=4, start='00:00', end='23:00')
dt = datetime(2014, 7, 3, 22)
dt + bh

Expected:

datetime(2014, 7, 4, 3)

Jiang Yue added some commits May 14, 2019

@codecov

This comment has been minimized.

Copy link

commented May 14, 2019

Codecov Report

Merging #26381 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26381      +/-   ##
==========================================
- Coverage   91.68%   91.67%   -0.01%     
==========================================
  Files         174      174              
  Lines       50704    50704              
==========================================
- Hits        46488    46485       -3     
- Misses       4216     4219       +3
Flag Coverage Δ
#multiple 90.18% <100%> (ø) ⬆️
#single 41.19% <0%> (-0.14%) ⬇️
Impacted Files Coverage Δ
pandas/tseries/offsets.py 96.69% <100%> (ø) ⬆️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97.01% <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 304d8d4...fffac75. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented May 14, 2019

Codecov Report

Merging #26381 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26381      +/-   ##
==========================================
- Coverage   91.68%   91.68%   -0.01%     
==========================================
  Files         174      174              
  Lines       50704    50738      +34     
==========================================
+ Hits        46488    46518      +30     
- Misses       4216     4220       +4
Flag Coverage Δ
#multiple 90.19% <100%> (ø) ⬆️
#single 41.16% <0%> (-0.17%) ⬇️
Impacted Files Coverage Δ
pandas/tseries/offsets.py 96.69% <100%> (-0.01%) ⬇️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/compat/__init__.py 93.54% <0%> (-0.4%) ⬇️
pandas/plotting/_style.py 76.92% <0%> (-0.26%) ⬇️
pandas/plotting/_misc.py 38.23% <0%> (-0.23%) ⬇️
pandas/core/frame.py 97.02% <0%> (-0.12%) ⬇️
pandas/plotting/_converter.py 63.66% <0%> (-0.06%) ⬇️
pandas/core/groupby/generic.py 88.96% <0%> (-0.05%) ⬇️
pandas/core/dtypes/dtypes.py 96.65% <0%> (-0.02%) ⬇️
pandas/plotting/_core.py 83.89% <0%> (-0.02%) ⬇️
... and 9 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 304d8d4...61079a7. Read the comment docs.

Jiang Yue

@gfyoung gfyoung added the Timeseries label May 14, 2019

@gfyoung

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@jiangyue12392 : Thanks for submitting this! For those tests cases that you've added, what happens currently without your fixes?

@jiangyue12392

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@gfyoung The current code without fixes would return a wrong value if the adding of business hour to the timestamp results in an ending time landing in the next business time period. For example, if the working hour is 00:00 to 23:00 and 4 business hours are added at 22:00, the current code would return 02:00 in the next day instead of 03:00 the next business day.

@gfyoung

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@jiangyue12392 : I see. For clarify, could you show the output of those examples, using a format like this (we use this format for our issue tracker):

Example

print("Hello World")

This outputs:

Hello World
@jiangyue12392

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@gfyoung Example inputs and outputs:
Input:

bh = BusinessHour(n=4, start='00:00', end='23:00')
dt = datetime(2014, 7, 3, 22)
dt + bh

Expected:

datetime(2014, 7, 4, 3)

Current Output:

datetime(2014, 7, 4, 2)
@gfyoung

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Got it. Can you add a whatsnew entry as well?

@jiangyue12392

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@gfyoung whatsnew entry added

@gfyoung

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@jiangyue12392 : A whatsnew entry means adding a bullet point to this file:

https://github.com/pandas-dev/pandas/blob/master/doc/source/whatsnew/v0.25.0.rst

I recommend adding it under the Timedelta section.

Jiang Yue
@jiangyue12392

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@gfyoung new entry added

Jiang Yue
@gfyoung
Copy link
Member

left a comment

@gfyoung gfyoung added the Bug label May 15, 2019

@@ -285,7 +285,7 @@ Timedelta

- Bug in :func:`TimedeltaIndex.intersection` where for non-monotonic indices in some cases an empty ``Index`` was returned when in fact an intersection existed (:issue:`25913`)
- Bug with comparisons between :class:`Timedelta` and ``NaT`` raising ``TypeError`` (:issue:`26039`)
-
- Bug in adding of business hour to the timestamp with an ending time landing in the next business time period (:pull:`26381`)

This comment has been minimized.

Copy link
@mroeschke

mroeschke May 15, 2019

Member

Bug when adding or subtracting a :class:`BusinessHour` to a :class:`Timestamp` with the resulting time landing in a following or prior day respectively (:issue:`26381`)

@jreback jreback added this to the 0.25.0 milestone May 15, 2019

@jreback

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

looks fine, ex- @mroeschke comment on the whatsnew.

@@ -1287,6 +1287,23 @@ def test_opening_time(self, case):
datetime(2014, 7, 7, 19, 30): datetime(2014, 7, 5, 4, 30),
datetime(2014, 7, 7, 19, 30, 30): datetime(2014, 7, 5, 4, 30, 30)}))

# long business hours (see gh-26381)
apply_cases.append((BusinessHour(n=4, start='00:00', end='23:00'), {

This comment has been minimized.

Copy link
@jreback

jreback May 15, 2019

Contributor

would love a separate PR to split up test_offsets.py a bit :->

Jiang Yue
@jreback

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

lgtm. ping on green.

@jiangyue12392

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@TomAugspurger TomAugspurger merged commit ebe6b91 into pandas-dev:master May 17, 2019

11 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20190516.5 succeeded
Details
pandas-dev.pandas (Checks_and_doc) Checks_and_doc succeeded
Details
pandas-dev.pandas (Linux py35_compat) Linux py35_compat succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow) Linux py36_locale_slow succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow_old_np) Linux py36_locale_slow_old_np succeeded
Details
pandas-dev.pandas (Linux py37_locale) Linux py37_locale succeeded
Details
pandas-dev.pandas (Linux py37_np_dev) Linux py37_np_dev succeeded
Details
pandas-dev.pandas (Windows py36_np15) Windows py36_np15 succeeded
Details
pandas-dev.pandas (Windows py37_np141) Windows py37_np141 succeeded
Details
pandas-dev.pandas (macOS py35_macos) macOS py35_macos succeeded
Details
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Thanks @jiangyue12392, nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.