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

open_at_time fix #166

Merged
merged 4 commits into from Jan 26, 2022
Merged

open_at_time fix #166

merged 4 commits into from Jan 26, 2022

Conversation

Stryder-Git
Copy link
Collaborator

This would be my proposed solution for #164, including an update to the tests.

@rsheftel, I think that it would be better to raise an error if a timestamp is passed that falls outside of the range covered by the schedule instead of returning False. Some tests are failing because I already implemented that. I wanted to see if you agree before adjusting those tests?

Thanks

@rsheftel
Copy link
Owner

I am not sure I understand, if the schedule defines an open and close time, then if the open_at_time() is called with a time outside the open hours, shouldn't it return False? Isn't that the purpose of the open_at_time() to know if a time is open or not?

@Stryder-Git
Copy link
Collaborator Author

Currently, if a schedule that has been calculated from "2001-01-08" to "2001-01-10" is used in open_at_time, with a timestamp before or after those dates (not covered by the schedule), it will return False.

I am suggesting that it may be better to raise an error in that situtaion:

import pandas_market_calendars as mcal
nyse = mcal.get_calendar("NYSE")

sched = cal.schedule("2001-01-08", "2001-01-10")
sched
                         market_open              market_close
2001-01-08 2001-01-08 14:30:00+00:00 2001-01-08 21:00:00+00:00
2001-01-09 2001-01-09 14:30:00+00:00 2001-01-09 21:00:00+00:00
2001-01-10 2001-01-10 14:30:00+00:00 2001-01-10 21:00:00+00:00


nyse.open_at_time(sched, "2001-01-10 00:00:00+0000") # covered by the schedule
False  # -> it is not open

nyse.open_at_time(sched, "2001-01-15 00:00:00+0000") # not covered by the schedule
False  # -> raise error ?

@rsheftel
Copy link
Owner

@Stryder-Git ok that makes sense and I agree we should implement this change.

@coveralls
Copy link

coveralls commented Jan 25, 2022

Coverage Status

Coverage remained the same at 96.217% when pulling d5ef5ec on Stryder-Git:open_at_time into 930eecd on rsheftel:master.

@Stryder-Git
Copy link
Collaborator Author

This is good to go but I will leave the versioning to you if that is fine? Thanks.

@rsheftel
Copy link
Owner

Works for me. Will push a new version to PyPi shortly

@rsheftel rsheftel merged commit e485b4e into rsheftel:master Jan 26, 2022
@Stryder-Git Stryder-Git deleted the open_at_time branch January 26, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants