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

BUG: Apply latest adjustment for minute `1d` #1256

Merged
merged 2 commits into from Jun 7, 2016

Conversation

Projects
None yet
3 participants
@ehebert
Member

ehebert commented Jun 3, 2016

Fix behavior in minute mode history with frequency 1d, where on the
day immediately following an adjustment action, the overnight adjustment
would not apply. (However the adjustment would be applied after a 1 day
lag.)

The root cause of the bug was that the history data for minute mode when
using 1d stitches together a sliding window of the daily data for
previous and the current minute. That daily data sliding window and
corresponding adjustments was being read as if the data was being viewed
from on the last day of the window; however in this case the data is
being viewed from the day after the window has completed. The difference
in view points requires the adjustments to popped and applied by the
adjusted array one index earlier. The fix uses the extra_slot value as
signifier on whether the data is being viewed on the following day and
then accordingly adjusts the index of the mulitpy object.

Also, change the split and merger test data ratios to have different values,
to ensure that different adjustment values are applied; as opposed to
doubling up on just one of the values.

@@ -126,6 +127,13 @@ def _get_adjustments_in_range(self, asset, dts, field):
The days for which adjustment data is needed.
field : str
OHLCV field for which to get the adjustments.
is_perspective_after : bool
see: _ensure_sliding_window
If True, the index at which the Mulitply object should be popped

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

some grammatical and spelling issues here, maybe from stitching together different versions of the docstring.

maybe remove the value before the time at which the adjustment is marked ? and clean up the typos.

@@ -176,18 +190,22 @@ def _get_adjustments_in_range(self, asset, dts, field):
ratio = s[1]
if start < dt <= end:
end_loc = dts.searchsorted(dt)
adj_loc = end_loc

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

out of scope here, but the work done in lines 187-190 should probably be moved into this if block, to avoid doing unnecessary work.

This comment has been minimized.

@ehebert

ehebert Jun 3, 2016

Member

Agreed, can follow up with that.

@@ -207,6 +225,13 @@ def _ensure_sliding_windows(self, assets, dts, field):
in the calendar.
field : str
The OHLCV field for which to retrieve data.
is_perspective_after : bool
True if the is window being viewed immediately after the last dt

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

typo - looks like is is in the wrong place?

is_perspective_after : bool
True if the is window being viewed immediately after the last dt
in the sliding window.
False if the window is being last dt.

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

another typo here: is being last dt

@@ -278,13 +305,22 @@ def history(self, assets, dts, field):
in the calendar.
field : str
The OHLCV field for which to retrieve data.
is_perspective_after : bool
True if the is window being viewed immediately after the last dt

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

same typos as above

@@ -142,30 +150,36 @@ def _get_adjustments_in_range(self, asset, dts, field):
dt = m[0]
if start < dt <= end:
end_loc = dts.searchsorted(dt)
adj_loc = end_loc
if is_perspective_after:
adj_loc -= 1

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

can you add a comment explaining why -=1 is the right thing to do here?

This comment has been minimized.

@ehebert
@jbredeche

This comment has been minimized.

Member

jbredeche commented Jun 3, 2016

looks good, bunch of docstring issues and a request for some more comments in the main area of change.

@ehebert ehebert force-pushed the fix-history-daily-freq-in-minute branch from 34a0a3f to 2568ae7 Jun 3, 2016

@ehebert

This comment has been minimized.

Member

ehebert commented Jun 3, 2016

Fixed docstrings and moved the ratio adjustment.

@@ -126,6 +127,12 @@ def _get_adjustments_in_range(self, asset, dts, field):
The days for which adjustment data is needed.
field : str
OHLCV field for which to get the adjustments.
is_perspective_after : bool
see: `USEquityHistoryLoader.history`
If True, the index at which the Mulitply object is registered to

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

This is pretty hard to parse, I've read it five times and still am not totally sure what you are saying. Can you break it up into smaller sentences?

also, typo "Mulitply"

is_perspective_after : bool
True if the window is being viewed immediately after the last dt
in the sliding window.
False if the window is being last dt.

This comment has been minimized.

@jbredeche

jbredeche Jun 3, 2016

Member

typo "is being last dt"

@ehebert ehebert force-pushed the fix-history-daily-freq-in-minute branch 2 times, most recently from d6e5a51 to 2df2e50 Jun 6, 2016

This flag is used for handling the case where the last dt in the
requested window immediately precedes a corporate action, e.g.:
- is_perpsective_after is True

This comment has been minimized.

@jbredeche

jbredeche Jun 6, 2016

Member

minor typo, but docs look much better, thanks!

@ehebert ehebert force-pushed the fix-history-daily-freq-in-minute branch from 2df2e50 to ef124ec Jun 6, 2016

@coveralls

This comment has been minimized.

coveralls commented Jun 6, 2016

Coverage Status

Coverage decreased (-0.03%) to 82.7% when pulling ef124ec on fix-history-daily-freq-in-minute into af38b02 on master.

ehebert added some commits Jun 2, 2016

BUG: Apply latest adjustment for minute `1d`
Fix behavior in minute mode history with frequency `1d`, where on the
day immediately following an adjustment action, the overnight adjustment
would not apply. (However the adjustment would be applied after a 1 day
lag.)

The root cause of the bug was that the history data for minute mode when
using `1d` stitches together a sliding window of the daily data for
previous  and the current minute. That daily data sliding window and
corresponding adjustments was being read as if the data was being viewed
from on the last day of the window; however in this case the data is
being viewed from the day after the window has completed. The difference
in view points requires the adjustments to popped and applied by the
adjusted array one index earlier. The fix uses the `extra_slot` value as
signifier on whether the data is being viewed on the following day and
then accordingly adjusts the index of the mulitpy object.

Also, change the split and merger test data ratios to have different values,
to ensure that different adjustment values are applied; as opposed to
doubling up on just one of the values.
MAINT: Only calc inverse ratio if it applies.
Avoid unneeded work by only calcultaing the inverse ratio when it
applies to the current range.

@ehebert ehebert force-pushed the fix-history-daily-freq-in-minute branch from ef124ec to 58467f9 Jun 7, 2016

@ehebert ehebert merged commit fd57705 into master Jun 7, 2016

2 checks passed

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

@ehebert ehebert deleted the fix-history-daily-freq-in-minute branch Jun 7, 2016

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