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

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

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

Conversation

ehebert
Copy link
Contributor

@ehebert ehebert commented Feb 22, 2018

So that the asset does not need to be re-retrieved from the asset finder, pass
assets to the dispatch reader's get_value.

Other Notes

This was noticed during an investigation into calling get_spot_value every bar.
There is a small performance benefit (<1% improvement in runtime for an algo that references 1000 assets every minute).
Regardless of the performance gain, making the dispatch reader's get_value expect an Asset type cleans up the use of the BarReader interface.

So that the asset does not need to be re-retrieved from the asset finder, pass
assets to the dispatch reader's `get_value`.
@coveralls
Copy link

coveralls commented Feb 22, 2018

Coverage Status

Coverage decreased (-0.07%) to 87.603% when pulling cb0a5e8 on pass-asset-to-dispatcher into 783cacd on master.

Copy link
Member

@richafrank richafrank left a comment

Choose a reason for hiding this comment

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

Thanks @ehebert . This looks good overall. I don't see anything obviously blocking, though I think the state of the world for passing assets versus sids to the various readers is still very confusing. Luckily I think the places that expect sids will work with assets instead, so I don't think this would break anything.

@@ -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?

@@ -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 ?

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants