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

Remove and zero out unused treasury curves #1910

Merged
merged 1 commit into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented Aug 11, 2017

Remove unused treasury curves attribute from the performance tracker, and zero out the treasury_period_return value rather than removing it so as not to require a schema change for backtest consumers.

@dmichalowicz dmichalowicz requested a review from ehebert Aug 11, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.3%) to 87.153% when pulling f57ea7a on remove-env-param into d3e0314 on master.

@ehebert

LGTM.
I have just one suggestion. At each place that we 'zero out' the value, we should add a comment about that value is expected to be zero, and that is there for protocol compatibility.

self._end_session,
self.trading_calendar,
)
self.treasury_period_return = 0

This comment has been minimized.

@ehebert

ehebert Aug 11, 2017

Member

Should we add a comment about how this is needed to retain API/protocol compatibility, but that it could removed?

@coveralls

This comment has been minimized.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.3%) to 87.233% when pulling fee2ee2 on remove-env-param into d3e0314 on master.

@dmichalowicz dmichalowicz force-pushed the remove-env-param branch from fee2ee2 to d7ef797 Aug 11, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.3%) to 87.233% when pulling d7ef797 on remove-env-param into d3e0314 on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.3%) to 87.233% when pulling d7ef797 on remove-env-param into d3e0314 on master.

@dmichalowicz dmichalowicz merged commit ced729d into master Aug 14, 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 remove-env-param branch Aug 14, 2017

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