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

MAINT: Modifies minute bars to use a dict of OHLC ratios #1428

Merged
merged 3 commits into from Aug 24, 2016

Conversation

Projects
None yet
3 participants
@yankees714
Contributor

yankees714 commented Aug 23, 2016

Instead of using a single ratio to scale up pricing for each sid before writing for bcolz, the writer now accepts a dict mapping each sid to the ratio to use. This arg is still optional, and we use the existing
OHLC_RATIO (1000) for every sid if no dict is passed.

This allows better handling of futures pricing data, where the required precision across root symbols is not consistent.

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.02%) to 86.179% when pulling edc6b01 on ohlc-ratios into b9a7e2f on master.

@@ -265,7 +274,8 @@ def read(cls, rootdir):
def __init__(
self,
ohlc_ratio,
default_ohlc_ratio,

This comment has been minimized.

@ehebert

ehebert Aug 23, 2016

Member

Not 100% sure we need to put the prefix of default here, but not going to block on it.

This comment has been minimized.

@yankees714

yankees714 Aug 23, 2016

Contributor

Was trying to make the distinction from ohlc_ratios_per_sid clear in the naming, but if it's overkill I'd be happy to simplify it.

if self._ohlc_ratios_per_sid is not None:
try:
return self._ohlc_ratios_per_sid[sid]
except KeyError:

This comment has been minimized.

@ehebert

ehebert Aug 23, 2016

Member

If the _default_ohlc_ratio is being used as a fallback/default, should it be returned here?

This comment has been minimized.

@yankees714

yankees714 Aug 23, 2016

Contributor

I opted here to share the return below for both the case where the sid is not present in the dict, and when no dict was passed to begin with. Do you think separate returns would be better?

This comment has been minimized.

@ehebert

ehebert Aug 23, 2016

Member

Never mind my comment, the pass does fall through correctly. It did read slightly off to me at first, but I believe passing on the KeyError is the correct way to go about this.

try:
return self._ohlc_inverses_per_sid[sid]
except KeyError:
pass

This comment has been minimized.

@ehebert

ehebert Aug 23, 2016

Member

Same as with ohclv_ratio_for_sid, should the _default_ohlc_inverse be returned here?

out[:len(where), i][where] = values[where]
if field != 'volume':
out *= self._ohlc_inverse
if field != 'volume':

This comment has been minimized.

@ehebert

ehebert Aug 23, 2016

Member

Why the change on how the data is written to the index?
I lean towards the original, where the assignment is done in one place, to minimize drift between OHLC and Volume with regard to the write indices.

This comment has been minimized.

@ehebert

ehebert Aug 23, 2016

Member

I understand now, since the ratio is per sid, the assignment had to be changed to within the loop.

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.149% when pulling affbb8c on ohlc-ratios into b9a7e2f on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.149% when pulling affbb8c on ohlc-ratios into b9a7e2f on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.149% when pulling affbb8c on ohlc-ratios into b9a7e2f on master.

2 similar comments
@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.149% when pulling affbb8c on ohlc-ratios into b9a7e2f on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.149% when pulling affbb8c on ohlc-ratios into b9a7e2f on master.

@yankees714 yankees714 force-pushed the ohlc-ratios branch from affbb8c to e8c7f88 Aug 24, 2016

MAINT: Modifies minute bars to use a dict of OHLC ratios
For scaling up pricing data before writing to bolz, the writer now
accepts a dict mapping each sid to the ratio to use. It still accepts a
single ratio as default_ohlc_ratio, which is used as a fallback if no
mapping exists for a given sid. The default is OHLC_RATIO (1000).

This allows better handling of futures pricing data, where the required
precision across root symbols is not consistent.

@yankees714 yankees714 force-pushed the ohlc-ratios branch from e8c7f88 to b3f1086 Aug 24, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.167% when pulling b3f1086 on ohlc-ratios into eba78ae on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.167% when pulling b3f1086 on ohlc-ratios into eba78ae on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.09%) to 86.129% when pulling 706a6bb on ohlc-ratios into eba78ae on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.09%) to 86.129% when pulling 706a6bb on ohlc-ratios into eba78ae on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.5%) to 85.735% when pulling 8e2617e on ohlc-ratios into eba78ae on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.05%) to 86.167% when pulling 8e2617e on ohlc-ratios into eba78ae on master.

@yankees714 yankees714 merged commit 012888f into master Aug 24, 2016

2 checks passed

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

@yankees714 yankees714 deleted the ohlc-ratios branch Aug 24, 2016

bartosh pushed a commit to bartosh/zipline that referenced this pull request Sep 27, 2016

MAINT: Modifies minute bars to use a dict of OHLC ratios (quantopian#…
…1428)

For scaling up pricing data before writing to bcolz, the writer now
accepts a dict mapping each sid to the ratio to use. It still accepts a
single ratio as default_ohlc_ratio, which is used as a fallback if no
mapping exists for a given sid. The default is OHLC_RATIO (1000).

This allows better handling of futures pricing data, where the required
precision across root symbols is not consistent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment