Rblp / Rblpapi

R package interfacing the Bloomberg API from https://www.bloomberglabs.com/api/
Other
166 stars 75 forks source link

returnAs=... support for bdh (closes #206) #334

Closed mtkerbeR closed 3 years ago

mtkerbeR commented 3 years ago

A first proposal of minor changes to code for function bdh() to return different types of objects - analogously to getTicks()

As this is one of my first contributions in R, please let me know about feedback/weaknesses of the proposed solutions, which I would be happy to take care of:

Two points worth mentioning:

  1. a new Option bdhType is introduced, to allow for backward-compatibility. Alternative would be to use blpType here as in getBars, however, if this value is set to a format which is not supported in bdh (like matrix, which is feasible in getBars) existing code might break. This could be catched by extending possible values in match.arg(returnAs, c("data.frame", "fts", "xts", "zoo", "data.table")) by getOption("blpType"), which would later on leads to a data.frame via the fallback option in switch if format is not supported.

  2. If some columns are non-numeric, types inheriting from zoo will convert numbers to characters for all columns (due to inheritance from matrix)

The code could be (unit)tested as follows - adapting existing test cases for bdh to the case of different values for parameter returnAs:

sec <- "TY1 Comdty"      
fields <- c("PX_LAST","OPEN_INT","FUT_CUR_GEN_TICKER") # probably also worth testing with fields of length 1, due to drop = FALSE
retAs <- c("data.frame", "fts", "xts", "zoo", "data.table")

res <- lapply(type, function(x) bdh(sec, fields, Sys.Date()-10, returnAs=x))

expect_true(all(sapply(seq_along(retAs), function(x) inherits(res[[x]], retAs[x]))), info = "checking return type")
expect_true(all(sapply(res, nrow) >= 5), info = "check return of five rows")
expect_true(all(sapply(res[sapply(res, function(x) !inherits(x, "zoo"))], ncol)==length(fields)+1), info = "check return of four columns for non-time series")
expect_true(all(sapply(res[sapply(res, function(x)  inherits(x, "zoo"))], ncol)==length(fields)),   info = "check return of four columns for time series")
expect_true(all(sapply(res, function(x) tail(colnames(x), length(fields)) == fields)), info = "check column names")
eddelbuettel commented 3 years ago

Oh, and my standard nag: can you please add a ChangeLog entry? Easy if you the One Editor Everybody Should Use (C-x 4 a from a file adds a stub for the file), still possibly for all others :)

eddelbuettel commented 3 years ago

There is a bit of a hash here as you added your two commits to a stale checkout of our repo. Looking at your repo fork:

image

eddelbuettel commented 3 years ago

And a simple rebase did not work. So I pulled a rip cord and

I suggest we fold this PR and I open a replacement. It contains your two commits so you still get credit.

Edit: See branch https://github.com/Rblp/Rblpapi/tree/feature/334_redo

mtkerbeR commented 3 years ago

Thanks a lot for your work on this.

Unfortunately, I could not connect the computer running Bloomberg to github due to security reasons, hence tried to find another solution - a bad one. Will update the changelog from another computer later on - which will hopefully work without additional problems.

Sorry for the trouble caused - thanks a lot for your patience!

eddelbuettel commented 3 years ago

You should really keep a current checkout of our repo, either from a fresh fork or an updated pull. So I think I will just close this and open a new one. Your two commits are preserved and you get credit from them -- just not from this PR but a parallel one.