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

Allow DataPortal.get_spot_value to accept multiple assets #1719

Merged
merged 1 commit into from Mar 25, 2017

Conversation

@dmichalowicz
Copy link
Contributor

@dmichalowicz dmichalowicz commented Mar 22, 2017

No description provided.

@dmichalowicz dmichalowicz requested a review from ehebert Mar 22, 2017
@dmichalowicz dmichalowicz force-pushed the vectorize-spot-value branch 2 times, most recently from 6faf544 to 47217b8 Mar 23, 2017
continue

if field not in BASE_FIELDS:
raise KeyError("Invalid column: " + str(field))

This comment has been minimized.

@dmichalowicz

dmichalowicz Mar 23, 2017
Author Contributor

I'll pull this check out of the for loop.

This comment has been minimized.

@ehebert

ehebert Mar 23, 2017
Contributor

👍

result = [
self.data_portal.get_spot_value(
assets=[equity, future],
field=field,

This comment has been minimized.

@ehebert

ehebert Mar 23, 2017
Contributor

Don't need to do with this patch. But a nice to have would be to support passing both the assets and the fields as iterables.

)
for field in expected.keys()
]
assert_almost_equal(array(list(expected.values())), result)

This comment has been minimized.

@ehebert

ehebert Mar 23, 2017
Contributor

If the expected values were a DataFrame this comparison would require less contortion.

if self._is_extra_source(asset, field, self._augmented_sources_map):
return self._get_fetcher_value(asset, field, dt)
assets_is_scalar = False
if isinstance(assets, (Asset, ContinuousFuture, str)):

This comment has been minimized.

@ehebert

ehebert Mar 23, 2017
Contributor

Could we use AssetConvertible or PricingDataAssociable here instead, which each have the Python string types registered (and handles Python 2/3).
Checking for just str is not sufficient.

continue

if field not in BASE_FIELDS:
raise KeyError("Invalid column: " + str(field))

This comment has been minimized.

@ehebert

ehebert Mar 23, 2017
Contributor

👍

self._get_minute_spot_value(asset, field, dt)
)

return spot_values[0] if assets_is_scalar else spot_values

This comment has been minimized.

@ehebert

ehebert Mar 23, 2017
Contributor

I have not profiled it, but always creating the out array may be a place to watch for a performance regression if/when get_spot_value is called in a tight loop.

A good way to check is to profile a 'buy and hold' type algorithm over a 500+ sized universe to make sure that that the the position tracker checks for the end of day values does not cause too much of a regression.
(Also the position tracker could be changed to pass a vector of assets, to take advantage of this change.)

This comment has been minimized.

@dmichalowicz

dmichalowicz Mar 23, 2017
Author Contributor

Ok, I'm going to make it so that we only create the out list if assets_is_scalar is False.

if data_frequency == "daily":
if field == "contract":
return self._get_current_contract(asset, session_label)
def get_single_asset_value(asset):

This comment has been minimized.

@dmichalowicz

dmichalowicz Mar 23, 2017
Author Contributor

I made this a helper function so that we can apply it to a list of assets only if we have to.

@dmichalowicz
Copy link
Contributor Author

@dmichalowicz dmichalowicz commented Mar 23, 2017

@ehebert Updated based on your comments.

@coveralls
Copy link

@coveralls coveralls commented Mar 23, 2017

Coverage Status

Coverage decreased (-0.004%) to 87.425% when pulling 3a52668 on vectorize-spot-value into 43d6004 on master.

@dmichalowicz dmichalowicz force-pushed the vectorize-spot-value branch from 3a52668 to 158d90a Mar 25, 2017
@coveralls
Copy link

@coveralls coveralls commented Mar 25, 2017

Coverage Status

Coverage decreased (-0.004%) to 87.425% when pulling 158d90a on vectorize-spot-value into 43d6004 on master.

@dmichalowicz dmichalowicz merged commit 0e8576e into master Mar 25, 2017
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
@dmichalowicz dmichalowicz deleted the vectorize-spot-value branch Mar 25, 2017
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

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