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 vectorized lookup_symbol. #1627

Merged
merged 3 commits into from Dec 28, 2016

Conversation

Projects
None yet
3 participants
@ssanderson
Member

ssanderson commented Dec 28, 2016

No description provided.

ENH: Add vectorized lookup_symbol.
Currently only supports one as_of date.c

@ssanderson ssanderson requested a review from llllllllll Dec 28, 2016

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2016

Coverage Status

Coverage increased (+0.009%) to 87.193% when pulling 876b9c7 on vectorized-symbol-map into 5025101 on master.

equities : list[Equity]
"""
memo = {}
out = []

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

should this just be a generator?

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2016

Member

Are you asking if the internal implementation should be a generator, or if the top-level interface should be a generator? I'm indifferent toward the former. w/r/t the latter, I can't think of a case where you'd want to call this and not have strict semantics, and returning a generator creates issues for using this with numpy/pandas, so I'd rather have this be eager.

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

I was talking about the top level impl, but this the numpy case makes sense.

'2015-01-02',
], utc=True),
symbols=[
['A.'], ['B.'], ['C.'], ['D.'],

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

Why do you have an empty sc ticker? Also, should this just fail to map?

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2016

Member

I'm not sure what you mean by "sc ticker".

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

shareclass, sorry.

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2016

Member

Ah. This is the current behavior of lookup_symbol, for better or worse. I was a bit surprised as well that these resolve, but I don't want to change the semantics here.

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

Okay, can you not test on that and just these A, B, C, and D then?

)
results = af.lookup_symbols(syms, dt, fuzzy=True)
assert_equal(results, af.retrieve_all([1, 3, 7]))

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

should you compare against a comprehension again?

fuzzy : bool, optional
Forwarded to ``lookup_symbol``.
Returns

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2016

Member

Maybe add a note saying that it is the same as the list comprehension but better for performance.

ssanderson added some commits Dec 28, 2016

TEST: Tweaks to vectorized symbol tests.
- Test against an empty list.
- Don't test empty share class lookups.
- Add another comprehension test for completeness.
@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 28, 2016

lgtm

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2016

Coverage Status

Coverage increased (+0.009%) to 87.193% when pulling e9b378f on vectorized-symbol-map into 5025101 on master.

@ssanderson ssanderson merged commit 54d78d0 into master Dec 28, 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 vectorized-symbol-map branch Dec 28, 2016

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