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

Updated getBars - support for adjustmentFollowDPDF #48

Closed
wants to merge 1 commit into from

Conversation

@joel23888
Copy link
Contributor

@joel23888 joel23888 commented Jul 15, 2015

This is a potential solution for adjusting intraday bar requests for corporate actions such as stock splits.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 14, 2015

@joel23888 I'd like to revisit this, but possibly as one option in a to-be-added new options or overrides vector. I'd rather not expand the interface with a bunch of toggles at the top-level. Thoughts?

@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Nov 15, 2015

Thanks. I think a vector approach would be good because there are currently several options. Would you want to default to adjusting for corporate actions or following DPDF?

Also, would you include gapFillInitialBar in the vector as well?

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 16, 2015

Yup. I'm out and about right now so can't take a detailed look right now but a single (named) vector may do, potentially also overrides.

Might be best to list a few options and have test function or two. So your help in this is greatly appreciated. ;-)

@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Nov 17, 2015

Happy to help, but I will have to look at this in more detail next week. Initial thoughts:

  • From the BBG API developer's guide the intraday bar request does not seem to have any overrides. So it would seem just a named vector for the options to be passed.
  • It makes sense to include gapFillInitialBar in the vector. How do we avoid breaking code already using this option explicitly?
  • For testing these, one relevant check to see if it is working is around corporate actions like stock splits. A recent example would be a 10 for 1 split in 8956 (Japanese stock), effective 10/28/15.
@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 17, 2015

Quickly:

  • nice, extracting names (for keys) and values is then trivial
  • we can keep it as a final (named) argument, test for being present (ie missing() will be FALSE) and then warn; the positional use may well die; that is a pill we may have to swallow
  • can you spec two quick call, with and without the option, to see how it affects the return data pre/post split?
@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Nov 30, 2015

@eddelbuettel I am busy with other things at the moment but I will return to this when things free up.

@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Dec 4, 2015

@eddelbuettel I can take a look at this - for the two calls you mentioned do you mean pick a corporate action and show expected behavior?

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Dec 4, 2015

@joel23888 yes, I guess my idea was to show a call without the option set and then one with it set so that we see in changed output that it was actually affecting the outcome as planned.

@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Dec 4, 2015

@eddelbuettel I have something working with an options vector (and remove the boolean paramter gapFillInitialBar) but no defaults. I think by default the behavior should reflect gapFillInitialBar=TRUE and adjustmentFollowDPDF=TRUE (after testing if it not passed then it will not adjust by default). What do you think and if you agree what is the preferred way to default?

@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Dec 4, 2015

@eddelbuettel You can see some of the tests I ran on this at:
https://github.com/joel23888/Rblpapi/commit/22d46985e578e1c4fad48a6e21102381131276bc

Let me know on the defaults and I'll make any adjustments and submit a new pull request.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Dec 4, 2015

The tests look good. (The commit is "dirty" as you included dll files...) A new (or modified) pull request would be good, this moves in the right direction!

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Dec 4, 2015

Can you make that a new or updated pull request? I can always clean up / adjust afterwards.

@joel23888
Copy link
Contributor Author

@joel23888 joel23888 commented Dec 5, 2015

Submitted a new one. Or let me know if you prefer me to clean up on Monday. Whatever works for you.

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

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