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

Still hitting NoDataBeforeDate edge cases #1894

Merged
merged 2 commits into from Jul 28, 2017

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented Jul 28, 2017

@yankees714 From what we discussed yesterday. The failing contract was 'EL', which is on a quarterly cycle.

@dmichalowicz dmichalowicz requested a review from yankees714 Jul 28, 2017

@@ -51,6 +51,7 @@ march_cycle_delivery_predicate = partial(delivery_predicate,
set(['H', 'M', 'U', 'Z']))
CHAIN_PREDICATES = {
'EL': march_cycle_delivery_predicate,

This comment has been minimized.

@yankees714

👍

tc = self.trading_calendar
trading_day = tc.day
prev = dt - trading_day
get_value = self.session_reader.get_value

This comment has been minimized.

@yankees714

yankees714 Jul 28, 2017

Contributor

The changes in this file are just cosmetic, correct?

This comment has been minimized.

@dmichalowicz

dmichalowicz Jul 28, 2017

Contributor

These four lines that moved are, but below I had to change dt to prev because prev is the actual day that we are calling get_value on.

@coveralls

This comment has been minimized.

coveralls commented Jul 28, 2017

Coverage Status

Coverage remained the same at 87.511% when pulling e9d02b4 on third-times-the-charm into ee42bd1 on master.

2 similar comments
@coveralls

This comment has been minimized.

coveralls commented Jul 28, 2017

Coverage Status

Coverage remained the same at 87.511% when pulling e9d02b4 on third-times-the-charm into ee42bd1 on master.

@coveralls

This comment has been minimized.

coveralls commented Jul 28, 2017

Coverage Status

Coverage remained the same at 87.511% when pulling e9d02b4 on third-times-the-charm into ee42bd1 on master.

# instead of to 'dt' is because we need to get each contract's volume
# on the previous day, so we need to make sure the back contract exists
# on 'prev' in order to call 'get_value' below.
if dt > front_contract.auto_close_date:

This comment has been minimized.

@dmichalowicz

dmichalowicz Jul 28, 2017

Contributor

@yankees714 Just FYI, I made this change as you pointed out to be > instead of >= (and Jamie confirmed), and updated the tests accordingly (basically just moving rolls up by a day where necessary).

This comment has been minimized.

@yankees714

yankees714 Jul 28, 2017

Contributor

Sounds good, thanks for the comment!

@coveralls

This comment has been minimized.

coveralls commented Jul 28, 2017

Coverage Status

Coverage remained the same at 87.511% when pulling 3427421 on third-times-the-charm into ee42bd1 on master.

@dmichalowicz dmichalowicz merged commit 8a886b8 into master Jul 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dmichalowicz dmichalowicz deleted the third-times-the-charm branch Jul 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment