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.

Sign up for GitHub

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

Jump to bottom

MAINT: Pass asset instead of sid to dispatch reader. #2114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

MAINT: Pass asset instead of sid to dispatch reader. #2114

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea439bc
MAINT: Pass asset instead of sid to dispatch reader.
Feb 22, 2018
cb0a5e8
MAINT: Remove now unneeded calls to retrieve_asset.
Feb 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions zipline/assets/roll_finder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def get_rolls(self, root_symbol, start, end, offset):
else:
first = front
first_contract = oc.sid_to_contract[first]
rolls = [((first_contract >> offset).contract.sid, None)]
rolls = [((first_contract >> offset).contract, None)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about the change in return value of this function (get_rolls),

  1. Should we change the docstring for rolls above?
  2. All the unit tests compare the return value to ints - should we change them or add comments?
  3. The call site in VolumeRollFinder.get_contract_center thinks it's still getting sids and calls retrieve_asset on them.
  4. Should we rename sid to asset in the unpacking of rolls from the call sites in [ContinuousFutureMinuteBarReader|ContinuousFutureSessionBarReader].load_raw_arrays like you did in history_loader.py below ?

tc = self.trading_calendar
sessions = tc.sessions_in_range(tc.minute_to_session_label(start),
tc.minute_to_session_label(end))
Expand All @@ -115,7 +115,7 @@ def get_rolls(self, root_symbol, start, end, offset):
session = sessions[-1]

while session > start and curr is not None:
front = curr.contract.sid
front = curr.contract
back = rolls[0][0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the uses of front and back, I notice that the parameter to BarReader.get_value and its various subclasses is still sid : int, but seems like some subclasses use it as an Asset or ContinuousFuture instance. Not easy to tell whether consumers of these contracts want the sid or the instance. The obvious cases do look to work with either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to that, any concerns that BarReader.get_value looks to expect sids generally, while AssetDispatchBarReader.get_value now expects Assets?

prev_c = curr.prev
while session > start:
Expand All @@ -127,7 +127,7 @@ def get_rolls(self, root_symbol, start, end, offset):
# TODO: Instead of listing each contract with its roll date
# as tuples, create a series which maps every day to the
# active contract on that day.
rolls.insert(0, ((curr >> offset).contract.sid, session))
rolls.insert(0, ((curr >> offset).contract, session))
break
session = prev
curr = curr.prev
Expand Down
6 changes: 3 additions & 3 deletions zipline/data/data_portal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ def _get_minute_spot_value(self, asset, column, dt, ffill=False):

if not ffill:
try:
return reader.get_value(asset.sid, dt, column)
return reader.get_value(asset, dt, column)
except NoDataOnDate:
if column != 'volume':
return np.nan
Expand All @@ -646,7 +646,7 @@ def _get_minute_spot_value(self, asset, column, dt, ffill=False):
try:
# Optimize the best case scenario of a liquid asset
# returning a valid price.
result = reader.get_value(asset.sid, dt, column)
result = reader.get_value(asset, dt, column)
if not pd.isnull(result):
return result
except NoDataOnDate:
Expand All @@ -662,7 +662,7 @@ def _get_minute_spot_value(self, asset, column, dt, ffill=False):
# no last traded dt, bail
return np.nan

result = reader.get_value(asset.sid, query_dt, column)
result = reader.get_value(asset, query_dt, column)

if (dt == query_dt) or (dt.date() == query_dt.date()):
return result
Expand Down
3 changes: 1 addition & 2 deletions zipline/data/dispatch_bar_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ def last_available_dt(self):
def first_trading_day(self):
return max(r.first_trading_day for r in self._readers.values())

def get_value(self, sid, dt, field):
asset = self._asset_finder.retrieve_asset(sid)
def get_value(self, asset, dt, field):
r = self._readers[type(asset)]
return r.get_value(asset, dt, field)

Expand Down
8 changes: 4 additions & 4 deletions zipline/data/history_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,14 +221,14 @@ def _get_adjustments_in_range(self, cf, dts, field):
adjs = {}

for front, back in sliding_window(2, rolls):
front_sid, roll_dt = front
back_sid = back[0]
front_contract, roll_dt = front
back_contract = back[0]
dt = tc.previous_session_label(roll_dt)
if self._frequency == 'minute':
dt = tc.open_and_close_for_session(dt)[1]
roll_dt = tc.open_and_close_for_session(roll_dt)[0]
partitions.append((front_sid,
back_sid,
partitions.append((front_contract,
back_contract,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the calls to retrieve_asset on the unpacked partition below?

dt,
roll_dt))
for partition in partitions:
Expand Down