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: Clear up naming and logic in resample close. #1728

Merged
merged 1 commit into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@ehebert
Member

ehebert commented Mar 28, 2017

  • Instead of maintaining a separate j value, set the bounds of the range so
    that i is the values emitted by the range.
  • Change close_loc to prev_close_loc since the market close location is used
    to ensure that the data index stops at the market open if the entire day is nans.
  • Change the setting of loc to be done before the loop which check for nans,
    instead of setting to the previous close loc at the end of the loop.

This prepares for a separate fix to prevent out of bounds access when the first
session has nans for all minutes.

Notes

This does not fix #1721

The following is a potential fix:

--- a/zipline/data/_resample.pyx
+++ b/zipline/data/_resample.pyx
@@ -82,11 +82,13 @@ cpdef void _minute_to_session_close(intp_t[:] close_locs,
         else:
             prev_close_loc = -1
         loc = close_locs[i]
-        val = data[loc]
-        while isnan(val) and loc > prev_close_loc:
-            loc -= 1
+        while loc > prev_close_loc:
             val = data[loc]
-        out[i] = val
+            if not isnan(val):
+                out[i] = val
+                break
+            else:
+                loc -= 1

@dmichalowicz is working on a failing test (e.g. where the first session has all nans) and applying the fix.

MAINT: Clear up naming and logic in resample close.
- Instead of maintaining a separate `j` value, set the bounds of the range so
that `i` is the values emitted by the range.
- Change `close_loc` to `prev_close_loc` since the market close location is used
to ensure that the data index stops at the market open if the entire day is
nans.
- Change the setting of `loc` to be done before the loop which check for nans,
instead of setting to the previous close loc at the end of the loop.

This prepares for a separate fix to prevent out of bounds access when the first
session has nans for all minutes.

@ehebert ehebert force-pushed the rework-resample-close branch from 4fbc2e0 to ed62d8a Mar 28, 2017

@ehebert

This comment has been minimized.

Member

ehebert commented Mar 28, 2017

The branch and PR comment have been with changes to when the loc value is set in the loop.

@ehebert ehebert requested a review from dmichalowicz Mar 28, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 28, 2017

Coverage Status

Coverage remained the same at 87.486% when pulling ed62d8a on rework-resample-close into f717e77 on master.

@coveralls

This comment has been minimized.

coveralls commented Mar 28, 2017

Coverage Status

Coverage remained the same at 87.486% when pulling ed62d8a on rework-resample-close into f717e77 on master.

@dmichalowicz

Looks good to me.

@ehebert ehebert merged commit f736169 into master Mar 28, 2017

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 rework-resample-close branch Mar 28, 2017

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