Skip to content

BUG: BDay() offsetting not behaving as expected #38324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2020
Merged

BUG: BDay() offsetting not behaving as expected #38324

merged 1 commit into from
Dec 18, 2020

Conversation

theoniko
Copy link
Contributor

@theoniko theoniko commented Dec 5, 2020 •
edited
Loading

arw2019
arw2019 reviewed Dec 5, 2020
View reviewed changes
Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more concise way to construct the expected result?

@theoniko theoniko force-pushed the theoniko-add-test-for-BDay-offsetting branch from 69d293a to e420c24 Compare December 6, 2020 07:53
@theoniko
Copy link
Contributor Author

theoniko commented Dec 6, 2020

Hello @arw2019,
I could make the expected have less timestamp by adjusting the date_range to smaller range. Would that be enough? |
Do you have any specific suggestion? This is my first contribution in this project so my proposed solution might not be
the most optimized.

@theoniko theoniko force-pushed the theoniko-add-test-for-BDay-offsetting branch 2 times, most recently from 1bf0549 to 63e97b2 Compare December 6, 2020 08:23
@theoniko theoniko mentioned this pull request Dec 9, 2020
Closed
arw2019
arw2019 reviewed Dec 9, 2020
View reviewed changes
Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm other than comment

pandas/tests/tseries/offsets/test_offsets.py
idx = pd.date_range("2010/02/01", "2010/02/15", freq="6H")
t1 = idx + BDay(offset=pd.Timedelta(3, unit="H"))

expected = DatetimeIndex(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't feel strongly about this but is there not a more concise way to generate this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing the date range seems to be the way to go here. Maybe extend the freq too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@theoniko theoniko force-pushed the theoniko-add-test-for-BDay-offsetting branch from 63e97b2 to 2adaf77 Compare December 12, 2020 17:14
@jreback jreback added the Frequency DateOffsets label Dec 13, 2020
@jreback jreback added this to the 1.3 milestone Dec 13, 2020
jreback
jreback requested changes Dec 13, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine if you can fixup the imports & ping on green.

pandas/tests/tseries/offsets/test_offsets.py Outdated
@@ -19,6 +19,7 @@
from pandas.compat.numpy import np_datetime64_compat
from pandas.errors import PerformanceWarning

import pandas as pd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import are all crazy here, instead of adding can you just do

from pandas import DatetimeIndex, date_range, Series, read_pickle

and remove the other references

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@theoniko theoniko force-pushed the theoniko-add-test-for-BDay-offsetting branch 2 times, most recently from f6b3e12 to 96275af Compare December 18, 2020 09:13
@theoniko theoniko force-pushed the theoniko-add-test-for-BDay-offsetting branch from 96275af to d20e166 Compare December 18, 2020 09:22
@theoniko theoniko requested a review from jreback December 18, 2020 09:52
@theoniko
Copy link
Contributor Author

theoniko commented Dec 18, 2020 •
edited
Loading

There is only a failing travis check but it does not seem to come from the pr changes. Is there a way to re-rerun only this check?
Also, note before editing the import in the file all the checks were green.

@jreback jreback merged commit ac7755e into pandas-dev:master Dec 18, 2020
@jreback
Copy link
Contributor

jreback commented Dec 18, 2020

thanks @theoniko

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
@theoniko @luckyvs1
BUG: BDay() offsetting not behaving as expected (pandas-dev#38324)
caa2de5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arw2019 arw2019 arw2019 left review comments

@phofl phofl phofl left review comments

@jreback jreback jreback approved these changes

@jbrockmendel jbrockmendel Awaiting requested review from jbrockmendel

Assignees
No one assigned
Labels
Frequency DateOffsets
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

BUG: BDay() offsetting not behaving as expected
5 participants
@theoniko @jreback @jbrockmendel @arw2019 @phofl