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

Check contract exists when using futures daily bar reader #1892

Merged
merged 1 commit into from Jul 20, 2017

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented Jul 18, 2017

The VolumeRollFinder failed get the active contract when using a daily bar reader.

@coveralls

This comment has been minimized.

coveralls commented Jul 18, 2017

Coverage Status

Coverage decreased (-0.009%) to 87.483% when pulling 99de180 on rolls-daily-bar-fix into 15fe38c on master.

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

@coveralls

This comment has been minimized.

coveralls commented Jul 19, 2017

Coverage Status

Coverage increased (+0.08%) to 87.569% when pulling f10461f on rolls-daily-bar-fix into 15fe38c on master.

@coveralls

This comment has been minimized.

coveralls commented Jul 19, 2017

Coverage Status

Coverage increased (+0.1%) to 87.615% when pulling 2ea6cea on rolls-daily-bar-fix into 15fe38c on master.

@coveralls

This comment has been minimized.

coveralls commented Jul 19, 2017

Coverage Status

Coverage increased (+0.1%) to 87.615% when pulling 2ea6cea on rolls-daily-bar-fix into 15fe38c on master.

@yankees714

LGTM, just had a few non-blocking suggestions.

@@ -90,6 +90,7 @@ def get_rolls(self, root_symbol, start, end, offset):
tc.minute_to_session_label(end))
freq = sessions.freq
if first == front:
# Compute at least one roll date no matter what.

This comment has been minimized.

@yankees714

yankees714 Jul 20, 2017

Contributor

Thanks for this comment, mind adding a little more detail on how this follows from the code? I still have to think about it for a while even though I've had a lot of exposure to this

This comment has been minimized.

@dmichalowicz

dmichalowicz Jul 20, 2017

Contributor

I'll try my best 😬

@@ -1058,6 +1058,8 @@ class level fixtures.
# options are: 'warn', 'raise', 'ignore'
BCOLZ_FUTURE_DAILY_BAR_INVALID_DATA_BEHAVIOR = 'warn'
_write_method_name = 'write'

This comment has been minimized.

@yankees714

yankees714 Jul 20, 2017

Contributor

Ah thanks for fixing, this was my fault because I was only using this fixture along with WithBcolzEquityDailyBarReader. Although now we won't be able to set this field separately between the two fixtures, maybe we should make this BCOLZ_FUTURE_DAILY_BAR_WRITE_METHOD_NAME?

2017-02-20 0 0 5000
...
2017-03-16 0 0 5000
2017-03-17 0 0 5000

This comment has been minimized.

@yankees714

yankees714 Jul 20, 2017

Contributor

This is great 🌈

# If we call 'get_rolls' with start and end dates that do not have any
# rolls between them, we should still expect the last roll date to be
# computed successfully.

This comment has been minimized.

@yankees714

yankees714 Jul 20, 2017

Contributor

Thoughts on making this its own test?

This comment has been minimized.

@dmichalowicz

dmichalowicz Jul 20, 2017

Contributor

Yeah probably worth making it its own

@dmichalowicz dmichalowicz force-pushed the rolls-daily-bar-fix branch from 2ea6cea to 75fc0fc Jul 20, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 20, 2017

Coverage Status

Coverage increased (+0.1%) to 87.615% when pulling 75fc0fc on rolls-daily-bar-fix into 15fe38c on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jul 20, 2017

Coverage Status

Coverage increased (+0.1%) to 87.615% when pulling 75fc0fc on rolls-daily-bar-fix into 15fe38c on master.

@dmichalowicz dmichalowicz merged commit 42ea84d into master Jul 20, 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 rolls-daily-bar-fix branch Jul 20, 2017

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