Skip to content
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

Daily adjustments #2089

Merged
merged 4 commits into from Feb 6, 2018
Merged

Daily adjustments #2089

merged 4 commits into from Feb 6, 2018

Conversation

@richafrank
Copy link
Member

@richafrank richafrank commented Jan 24, 2018

BUG: On daily data, yesterday's adjustments were being applied
to yesterday's data from today's perspective, but no adjustment
happened between yesterday and today. They happened before yesterday's
open.

@richafrank richafrank requested a review from ehebert Jan 24, 2018
@richafrank richafrank force-pushed the daily_adjustments branch from 3749969 to b03fd43 Jan 24, 2018
@coveralls
Copy link

@coveralls coveralls commented Jan 25, 2018

Coverage Status

Coverage increased (+0.01%) to 87.629% when pulling fbfba16 on daily_adjustments into e5082cb on master.

Copy link
Contributor

@ehebert ehebert left a comment

My recommendation is to fix get_adjustments instead of introducing a minute dt, during a daily data_frequency invocation of get_adjusted_value.

@@ -643,6 +640,9 @@ def get_adjusted_value(self, asset, field, dt,
data_frequency)

if isinstance(asset, Equity):
if data_frequency == 'daily':
dt = self.trading_calendar.session_open(dt)

This comment has been minimized.

@ehebert

ehebert Jan 26, 2018
Contributor

When in daily data frequency, I don't believe we should be introducing a dt which is not date normalized.

I think this bug shows that we should be adding a data_frequency parameter to get_adjustments, and making the query for get_adjustments aware of the data_frequency.

This comment has been minimized.

@ssanderson

ssanderson Jan 26, 2018
Contributor

I disagree with this recommendation. data_frequency is a algorithm-level concept; we shouldn't be pushing that down to the layers of the stack that are purely about data retention and manipulation. It's straightforward and context-independent to reason about what it means to say "give me data from time t0 adjusted as of time t1", whereas I have no intuition for what it means to say "give me data from time t0 at 'daily' data_frequency".

This comment has been minimized.

@ehebert

ehebert Jan 26, 2018
Contributor

@ssanderson you are right about keeping DataPortal free needing knowledge from the algorithm/backtest level data frequency. It had been a while since I've looked at this code, and had forgotten how important that is. Thanks for the admonition of keeping the levels separate.

However, I still believe that this is the wrong place to fix the application of adjustments when the dt and the perspective dt are on the same day.
The bug is that get_adjustments returns an adjustment when . I'll follow up with a suggestion how to fix that method.

@richafrank richafrank force-pushed the daily_adjustments branch 3 times, most recently from 6de7a87 to 8f1a1c3 Jan 26, 2018
@richafrank
Copy link
Member Author

@richafrank richafrank commented Jan 27, 2018

@ehebert Updated as we discussed. I think the test cases here cover the scenarios we talked through.

@richafrank richafrank force-pushed the daily_adjustments branch from 8f1a1c3 to 1e3d046 Feb 5, 2018
richafrank added 4 commits Jan 24, 2018
to yesterday's data from today's perspective, but no adjustment
happened between yesterday and today. They happened before yesterday's
open.
@richafrank richafrank force-pushed the daily_adjustments branch from 1e3d046 to fbfba16 Feb 6, 2018
@richafrank richafrank merged commit 70f8d14 into master Feb 6, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@richafrank richafrank deleted the daily_adjustments branch Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.