-
Notifications
You must be signed in to change notification settings - Fork 26
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
add support beqs #26
add support beqs #26
Conversation
This looks good, I pushed one commit to add some slight code formatting since I try to follow black code formatting conventions https://black.readthedocs.io/en/stable/ |
Just wanted to confirm you have ran this and it works as expected? I don't currently have access to a terminal so I'm unable to test the feature. |
src/blp/blp.py
Outdated
df = self.query(query, self.parser, self.collect_to_beqs) | ||
columns = ["security"] + sorted([col for col in df.columns if col != "security"]) | ||
df = ( | ||
df.sort_values("security").reset_index(drop=True).loc[:, columns].drop(["Ticker"], axis=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for dropping Ticker
? Is this a redundant piece of information that is returned by the Bloomberg api that is always equal to security
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly! However, if you prefer, we could just leave the Ticker information and let the user drop it if he wants to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I think that would be more consistent with other approaches in the library, where I try to pass through the Bloomberg response in a fairly unmodified way.
I have tested a few examples, and everything seems to be functioning well. However, I haven't conducted a systematic test. If you have specific tests you'd like me to run, feel free to share them, and I'll be happy to execute them. |
…dropping it. Second, don't sort columns on alphabetical order.
Hello, @matthewgilbert, following your advice, we are now preserving the 'Ticker' column rather than discarding it. Also, given EQS's way of displaying metrics and ranks side by side, I've taken out the part of the code that sorts columns alphabetically. Do you believe any further adjustments are necessary before merging? P.S. I have tested this with a private screen using a backdate and it functioned as expected. Additionally, it performed well on a public screen once the timeout was adjusted. |
Great, merged! Thanks for contributing. |
Hello!
Last week I opened an issue (#25) from my personal account. The issue relates to the support for beqs, and I believe I have addressed it in this pull request.
However, this is my first pull request ever, so I'm not entirely sure if everything is in order. Any feedback on that matter would be great!
Finally, if you think it's good to go, I can start work on some documentation for the new functionality.
Thanks!