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

Better date range #142

Merged
merged 22 commits into from
Aug 18, 2021
Merged

Better date range #142

merged 22 commits into from
Aug 18, 2021

Conversation

Stryder-Git
Copy link
Collaborator

Following the discussion in #138:
This is the enhanced version of calendar_utils.date_range, which has been replaced with the instance date_range of the new callable class _date_range.

The only test that needed to be changed is test_utils.test_date_range_w_breaks to allow for the new way of handling breaks.
The main reason for a different behaviour in some situations is that break_start is now considered the close of the first trading session and break_end the open of the second trading session of a trading day, which can lead to a different outcome when closed= None or force_close= True.

I also added two tests:
test_utils.test_date_range_exceptions: testing if expected exceptions are raised
test_utils.test_date_range_permutations: testing if all closed/force_close inputs lead to the expected outcome.
--> have a look at the last two assertions in test_date_range_permutations to see the new configuration that I mentioned in my last comment in #138

I also wrote a class docstring in calendar_utils._date_range, let me know if I should add/clarify anything.

@Stryder-Git
Copy link
Collaborator Author

After briefly having a look and updating all my packages, it seems that I get the same fails as in #141 when pandas > 1.2.5. But when I downgrade it to pandas==1.2.5, all the tests pass in my fork.

When running it with pandas==1.2.5, i also get:
"FutureWarning: Indexing a timezone-naive DatetimeIndex with a timezone-aware datetime is deprecated and will raise KeyError in a future version. Use a timezone-naive object instead.", which could be the problem

I will have a more thorough look when I find the time.

@rsheftel
Copy link
Owner

After briefly having a look and updating all my packages, it seems that I get the same fails as in #141 when pandas > 1.2.5. But when I downgrade it to pandas==1.2.5, all the tests pass in my fork.

When running it with pandas==1.2.5, i also get:
"FutureWarning: Indexing a timezone-naive DatetimeIndex with a timezone-aware datetime is deprecated and will raise KeyError in a future version. Use a timezone-naive object instead.", which could be the problem

I will have a more thorough look when I find the time.

@Stryder-Git , the problem had nothing to do with this PR but some change to functionality in pandas 1.2.5 that effected the tests (not the code). That has been fixed in master and released as v2.1

If you guys sync this PR with the most recent master all tests and Travis CI should pass, can you do that?

Also, at this point are there any open topics we need to solve before merging?

@coveralls
Copy link

coveralls commented Aug 16, 2021

Coverage Status

Coverage increased (+0.06%) to 95.316% when pulling b9d8696 on Stryder-Git:better_date_range into 6b8ed50 on rsheftel:master.

@Stryder-Git
Copy link
Collaborator Author

v2.1 is merged, all tests pass.

@rsheftel, If you agree with the changes I explained, this PR is good to go.

@rsheftel rsheftel merged commit c969e72 into rsheftel:master Aug 18, 2021
@Stryder-Git Stryder-Git deleted the better_date_range branch September 17, 2021 16:17
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.

3 participants