bds no longer returns data.frame, but list

[stellathecat]

library(Rblpapi) con <- blpConnect() data <- bds("GOOG US Equity", "TOP_20_HOLDERS_PUBLIC_FILINGS") class(data) [1] "list"

is this expected or will this be reversed? thank you!

sessionInfo() R version 4.1.2 (2021-11-01) Platform: x86_64-w64-mingw32/x64 (64-bit) Running under: Windows Server x64 (build 17763)

Matrix products: default

locale: [1] LC_COLLATE=German_Switzerland.1252 LC_CTYPE=German_Switzerland.1252 LC_MONETARY=German_Switzerland.1252 [4] LC_NUMERIC=C LC_TIME=German_Switzerland.1252

attached base packages: [1] stats graphics grDevices utils datasets methods base

other attached packages: [1] Rblpapi_0.3.11 devtools_2.4.3 usethis_2.1.3

loaded via a namespace (and not attached): [1] Rcpp_1.0.7 rstudioapi_0.13 magrittr_2.0.1 pkgload_1.2.4 R6_2.5.1 rlang_0.4.12
[7] fastmap_1.1.0 tools_4.1.2 pkgbuild_1.2.1 sessioninfo_1.2.2 cli_3.1.0 withr_2.4.3
[13] ellipsis_0.3.2 remotes_2.4.2 rprojroot_2.0.2 lifecycle_1.0.1 crayon_1.4.2 processx_3.5.2
[19] purrr_0.3.4 callr_3.7.0 fs_1.5.1 ps_1.6.0 fractional_0.1.3 curl_4.3.2
[25] testthat_3.1.1 memoise_2.0.1 glue_1.5.1 cachem_1.0.6 compiler_4.1.2 desc_1.4.0
[31] prettyunits_1.1.1 jsonlite_1.7.2

[mtkerbeR]

It looks like the return type of bds has changed between version 0.3.10 ...

library(Rblpapi)
packageVersion("Rblpapi")
#> [1] '0.3.10'

blpConnect()
sec <- bds("DAX Index", "INDX_MEMBERS")
class(sec)
#> [1] "data.frame"

and 0.3.11:

library(Rblpapi)
packageVersion("Rblpapi")
#> [1] '0.3.11'

blpConnect()
sec <- bds("DAX Index", "INDX_MEMBERS")
class(sec)
#> [1] "list"

If return type "data.frame" is preferred, I would happily provide a corresponding PR.

[eddelbuettel]

As I recall we tried to establish a preferred return (and set that to bdh). We may have omitted that in a cleanup for bds.

In any event, you can data.frame(bds("DAX Index", "INDX_MEMBERS")) now, and same for tibble or data.frame if you prefer.

Please do not rush off with a PR before we have consensus answers from John/Whit/myself.

[eddelbuettel]

From a glance, seems to / could depend on which branch of the switch in bds_Impl is hit? Odd.

[csrvermaak]

Thanks Dirk. The data.frame() workaround is a good stopgap. However, please spare a thought for the requirement for existing codebases that make use of the previous version of the package to have to change every bds() call. Due to practical implications and backward compatibility, would think it ideal to retain output type across version increments (except of course for good reasons).

[eddelbuettel]

Absolutely and understood. We try to never change (default) interface. This is a bug in 0.3.11 (and a side effect of fixing something else) but the question now is about the cleanest way forward.

If I may, it would help us if you all built from the git repo every now and then and tried release candidates in your settings. That change has been there for eight month it seems. We could help with Windows binaries (in a drat repo) if that is the difference maker.

[klin333]

The ideal end state for me is:

bds() interface should reserve the ability to handle multiple securities per request:

• While currently Rblpapi only supports 1 security per bds call, future improvements could easily allow multiple securities per bds call. I know previous issues #151 said users can just loop in user code, however, it is absurdly slow due to overhead associated with making a bloomberg request and the event processing loop. Future improvements to bds_impl can easily speed it up by 50 times or so by being able to process PARTIAL_RESPONSE events. Hence, we should reserve the ability of bds() to handle multiple securities per request. Note that I've been doing this for years now but cancelled my pull request #298 due to segfaults that I thought I introduced in multi-security bds, but was fixed lately where culprit turned out to be unrelated unprotected SEXPs.

both bds() and bdh() should return a list of data.frames irrespective of the number of securities requested. (this is purely a wish that i concede we won't get to)

• While it can be easy interactive use to return a data.frame when only 1 security is requested (bdh current behaviour, and the prior bds behaviour), it is inconsistent behaviour and user code often have to use a if else case to handle the two different bdh returned data types based on number of requested sescurities. This is highly not ideal.

In light of this, I have mixed feelings about commit 4d11c47f42079936bb24a2a08e57d04d32b48dab . It's intention is bad, because its commit message implied we gave up on ability to handle multiple securities per bds request. However, it's action (intended or not) is good, ie make it always a list of data.frames irrespective of number of requested securities. I believe it's too late to change to a list of data.frames now due to backwards compatibility (power users can and probably already always just wrap Rblpapi::bdh/s calls with their own wrapped functions to handle the return type inconsistency).

So, I propose we revert commit 4d11c47f42079936bb24a2a08e57d04d32b48dab. Doing this revert will get back to backward compatible behaviour of data.frame if only 1 security requested (for both bds and bdh), and a list of data.frames if multiple securities requested (for bdh currently, and bds though bds will currently crash on a stop check, but may be allowed in the future).

[johnlaing]

Agreed this is a bug, and it appears that @klin333 is correct to point to 4d11c47. This is easy to fix. But the fact that it happened exposes that we have no tests for bds. Something like this (from https://github.com/Rblp/Rblpapi/blob/0.3.12/inst/tinytest/test_bdh.R#L36) would have caught it:

expect_true(inherits(res, "data.frame"), info = "checking return type")

So we should add some basic tests at the same time.

Supporting multiple securities is a worthwhile endeavor, but separate from this. @klin333, feel free to take another shot at that (in a separate issue/PR) now that the pointer issues have been resolved. If implemented we should maintain the default single-security-single-data.frame behavior, but I would be in favor of an option to return a list of data.frames however many securities are passed.

[eddelbuettel]

Agree 100% with both of you. When I still had Bbg access, I ran unit tests against live data source. I now owe the project an update to switch to GH actions (I have student projects to mark this weekend, so give me til next week -- it is otherwise something I can add and test in an evening as I have done the transition to GitHub Actions for dozens of my other packages). When that is up then we can easily add unit test condition on an 'opt in' environment variable such as WITH_BBG (or whatever) and either of you tow (or @armstrtw) could run this. Sadly we will never be able to run it at GH.

As for the larger/bigger bite mentioned by @klin333: can do, but as @johnlaing mentioned preferably in a differnt PR and with a bit of care / patience. Also a lot of the inner code is getting old, some of it was added before Rcpp matured a bit. We have better facilities in many places now (including C++14 up to C++17) so some of the dynamic data gathering could be redone. Is it worth it? I don't know. It is always worth it for the person constrained by the current state and with enough chops and time to do something about it. Otherwise, things mostly work (cough, cough, unless years old bugs get fixed) and only better with fixes. So ... it all depends. As always.

[mtkerbeR]

@eddelbuettel Concerning your point on testing release candidates: I usually try out if changes to the code are compatible with exisiting code on my side/setting. Unfortunately, my code rarely uses function bds. And using bds exclusively in an interactive setting, I didn't notice the difference. Thanks a lot for offering Windows binaries to support this - for me, this wouldn't be required.

[eddelbuettel]

I wrote

I now owe the project an update to switch to GH actions

and I lied, because I apparently already took care of that in March :)

The unit tests have also been converted alread to tinytest so in order run tests we "only" (as before) need to set the environment variable RunRblpapitUnitTests to yes or else the test files exit:

edd@rob:~/git/rblpapi(master)$ grep Run inst/tinytest/test_*
inst/tinytest/test_bdh.R:.runThisTest <- Sys.getenv("RunRblpapiUnitTests") == "yes"
inst/tinytest/test_bdp.R:.runThisTest <- Sys.getenv("RunRblpapiUnitTests") == "yes"
inst/tinytest/test_examples.R:.runThisTest <- Sys.getenv("RunRblpapiUnitTests") == "yes"
inst/tinytest/test_getBars.R:.runThisTest <- Sys.getenv("RunRblpapiUnitTests") == "yes"
inst/tinytest/test_getTicks.R:.runThisTest <- Sys.getenv("RunRblpapiUnitTests") == "yes"
edd@rob:~/git/rblpapi(master)$ 

It does show, though, that we may not have a file test_bds.R so if some (who can test) wants to contribute one -- understanding tinytest should be pretty easy given the other files, and the tinytest documentation.

[eddelbuettel]

Please branch feature/simplify_argument which aims to address this. I can create a draft PR and/or a binary if that helps in testing.

[eddelbuettel]

This fix is now merged.