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

ENH: Add `SpecificAssets` filter. #1530

Merged
merged 4 commits into from Oct 9, 2016

Conversation

Projects
None yet
4 participants
@ssanderson
Member

ssanderson commented Oct 7, 2016

Adds a filter that matches a set of assets. Mainly useful for testing and debugging.

ENH: Add `SpecificAssets` filter.
Adds a filter that matches a set of assets.  Mainly useful for testing
and debugging.
@coveralls

This comment has been minimized.

coveralls commented Oct 7, 2016

Coverage Status

Coverage increased (+0.01%) to 86.816% when pulling f874e67 on add-specific-assets into eba02da on master.

@llllllllll

overall lgtm, 3 small questions.

window_length = 0
params = ('sids',)
@expect_types(assets=(list, tuple, np.ndarray))

This comment has been minimized.

@llllllllll

llllllllll Oct 8, 2016

Member

how do we get an ndarray, it looks like we will coerce it to a list in the coerce below.

This comment has been minimized.

@ssanderson

ssanderson Oct 9, 2016

Member

Removed the coercions here.

@expect_types(assets=(list, tuple, np.ndarray))
@coerce_types(assets=((list, np.ndarray, pd.Series), list))
def __new__(cls, assets):
sids = frozenset(asset.sid for asset in assets)

This comment has been minimized.

@llllllllll

llllllllll Oct 8, 2016

Member

why do we need to coerce this to a list to iterate over it?

@@ -820,3 +822,40 @@ def test_top_and_bottom_with_groupby_and_mask(self, dtype, seed):
},
mask=self.build_mask(permute(rot90(self.eye_mask(shape=shape)))),
)
class SpecificAssetsTestCase(WithSeededRandomPipelineEngine,

This comment has been minimized.

@llllllllll

llllllllll Oct 8, 2016

Member

why does this need a seeded random loader, we are not loading any values, just sids. Can we just create an engine with an exploading object for a loader?

This comment has been minimized.

@ssanderson

ssanderson Oct 9, 2016

Member

We could, but setting up the engine with sids and trading days is a little bit boilerplate-ey. I tend to use WithSeededRandomPipelineEngine when I just need an engine, but I don't care about the specific values it emits.

@justnpT

This comment has been minimized.

justnpT commented Oct 9, 2016

I would vote for adding docs about what specific assets are

@coveralls

This comment has been minimized.

coveralls commented Oct 9, 2016

Coverage Status

Coverage increased (+0.07%) to 86.872% when pulling 46a0e86 on add-specific-assets into eba02da on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 9, 2016

Coverage Status

Coverage increased (+0.07%) to 86.872% when pulling d0c10e0 on add-specific-assets into eba02da on master.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Oct 9, 2016

@justnpT I added another sentence to the docstring explaining when you would use SpecificAssets: https://github.com/quantopian/zipline/pull/1530/files#diff-9bcf584f1d4b53abe9f16f81e5ee0716R503.

@ssanderson ssanderson merged commit acc46f5 into master Oct 9, 2016

2 checks passed

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

@ssanderson ssanderson deleted the add-specific-assets branch Oct 9, 2016

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