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

Do not explicitly round asset prices #1788

Merged
merged 1 commit into from May 24, 2017

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented May 9, 2017

Previously, all asset prices were being rounded to 3 decimal places because this is the most precision we ever needed for equities. Some futures however have tick sizes with precision less than 0.001, so we don't want to cut off those price values.

Since we already cutoff the decimal places for equities to three places by default (https://github.com/quantopian/zipline/blob/master/zipline/data/minute_bars.py#L55) the explicit rounding when reading is unnecessary anywhere. And futures prices should be already cutoff to the appropriate precision according to their ohlc_ratios_per_sid value.

asset = cls.asset_finder.retrieve_asset(10000)
trading_calendar = cls.trading_calendars[asset.exchange]
trading_sessions = cls.trading_sessions[asset.exchange]
trading_calendar = cls.trading_calendars['us_futures']

This comment has been minimized.

@yankees714

yankees714 May 9, 2017

Contributor

We should be pass the Future type here to make it clear that we're getting the primary calendar for futures, and in case we ever want change it.

This comment has been minimized.

@yankees714

yankees714 May 9, 2017

Contributor

I think that would allow us to remove the override of TRADING_CALENDAR_STRS as well.

This comment has been minimized.

@dmichalowicz

dmichalowicz May 9, 2017

Contributor

Will do

@@ -58,6 +58,9 @@
OBJECT_DTYPES,
)
# Default number of decimal places used for rounding asset prices.
DEFAULT_ASSET_PRICE_DECIMALS = 3

This comment has been minimized.

@dmichalowicz

dmichalowicz May 9, 2017

Contributor

I'm not sure if this file is the best place for this constant to live.

@dmichalowicz dmichalowicz force-pushed the rounding-cutoff-2 branch from 5cfad61 to b9d3454 May 10, 2017

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented May 10, 2017

@yankees714 Ok this should be ready for another look.

# If no roll finders were given, fall back on always using a
# calendar roll finder. The only current use of this attribute is
# to extract the tick size of the center contract of continuous
# futures, which should not depend on roll dates anyway.

This comment has been minimized.

@yankees714

yankees714 May 12, 2017

Contributor

Isn't the center contract determined by roll dates?

This comment has been minimized.

@yankees714

yankees714 May 12, 2017

Contributor

Oh I see, you're saying that the tick size doesn't depend on the center contract.

rf = self._roll_finders[asset.roll_style]
contract_sid = rf.get_contract_center(
asset.root_symbol, reference_date, asset.offset,
)

This comment has been minimized.

@yankees714

yankees714 May 12, 2017

Contributor

Can you just use the asset finder for this?

contract_sid = self._asset_finder.get_ordered_contracts(
    asset.root_symbol).contract_before_auto_close(reference_date.value)

Seems like the offset probably doesn't matter?

This comment has been minimized.

@dmichalowicz

dmichalowicz May 12, 2017

Contributor

Yeah the offset doesn't matter. Then this way I don't need to set a _roll_finders attribute either.

@@ -227,6 +227,7 @@ def traverse(self,
offset,
window_length,
perspective_offset,
rounding_places=None,

This comment has been minimized.

@yankees714

yankees714 May 12, 2017

Contributor

So this means we don't want to round here?

This comment has been minimized.

@dmichalowicz

dmichalowicz May 23, 2017

Contributor

Correct, for instance in pipeline, if there's a price adjustment that causes prices to go out to more than 3 decimal places, I don't believe any rounding currently occurs.

@@ -40,6 +40,7 @@ cdef class AdjustedArrayWindow:
readonly Py_ssize_t window_length
Py_ssize_t anchor, max_anchor, next_adj
Py_ssize_t perspective_offset
object rounding_places

This comment has been minimized.

@yankees714

yankees714 May 12, 2017

Contributor

Can we add some explanation of this to the docstring?

@dmichalowicz dmichalowicz force-pushed the rounding-cutoff-2 branch 2 times, most recently from 2cdb9f9 to 489e22f May 23, 2017

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented May 23, 2017

@yankees714 Updated based on your latest comments

@@ -138,6 +145,8 @@ cdef class AdjustedArrayWindow:
new_out = asanyarray(self.data[anchor - self.window_length:anchor])
if view_kwargs:
new_out = new_out.view(**view_kwargs)
if self.rounding_places is not None and new_out.dtype is dtype('float64'):

This comment has been minimized.

@yankees714

yankees714 May 23, 2017

Contributor

Seems like np.issubdtype might be the more robust way to do this? Also any reason not to just import float64 directly?

This comment has been minimized.

@dmichalowicz

dmichalowicz May 23, 2017

Contributor

Good call on the np.issubdtype, will change.

Also any reason not to just import float64 directly?

I've seen throughout zipline we tend to favor using dtype('float64') as seen here, so I was just following that convention.

This comment has been minimized.

@yankees714

yankees714 May 23, 2017

Contributor

Got it, makes sense.

@yankees714

LGTM

@dmichalowicz dmichalowicz force-pushed the rounding-cutoff-2 branch from ca60577 to 41aa212 May 24, 2017

@coveralls

This comment has been minimized.

coveralls commented May 24, 2017

Coverage Status

Coverage increased (+0.05%) to 87.586% when pulling 41aa212 on rounding-cutoff-2 into 28060bf on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented May 24, 2017

Coverage Status

Coverage increased (+0.05%) to 87.586% when pulling 41aa212 on rounding-cutoff-2 into 28060bf on master.

@dmichalowicz dmichalowicz merged commit d760a84 into master May 24, 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 rounding-cutoff-2 branch May 24, 2017

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