Open edonnachie opened 1 year ago
Native pipe would restrict you to R>=4.1.0.
Yes, so I tend to go for some old-style coding here.
In theory, CRAN only supports the last two versions for binary builds. So at some point, we should be able to assume 4.1, but I don't think we're quite there yet.
For icd_lookup() and icd_browse(), the base R versions seems to be considerably quicker (see commit "ba27277406ec5394eab9a311ec5b99ed565fd24c"):
> microbenchmark::microbenchmark(
+ icd_lookup_base("I40"),
+ ICD10gm::icd_lookup("I40")
+ )
Unit: milliseconds
expr min lq mean median uq max neval
icd_lookup_base("I40") 58.27868 62.93969 75.52261 70.21354 77.59054 232.4084 100
ICD10gm::icd_lookup("I40") 91.51122 103.47907 118.29817 110.88956 125.94790 223.3503 100
> microbenchmark::microbenchmark(
+ icd_browse_base("I40", open_browser = FALSE),
+ ICD10gm::icd_browse("I40", open_browser = FALSE)
+ )
Unit: milliseconds
expr min lq mean median
icd_browse_base("I40", open_browser = FALSE) 9.188926 9.578174 10.93701 9.789944
ICD10gm::icd_browse("I40", open_browser = FALSE) 49.116336 51.738686 57.86907 53.456514
uq max neval
10.58747 30.25024 100
59.72987 90.34282 100
Dependency on dplyr, purrr and magrittr was primarily for convenience and simplicity. It is assumed that most users have these packages installed already, but package check (CI) can take longer than necessary because dependencies need to be installed.
It should be possible to replace tidyverse dependencies with base R, or even data.table (which might also bring big performance improvements).
This affects primarily icd_expand() and icd_history().
There is also no reason to keep the magrittr pipe. This could either be replaced with the new native pipe, or coded "old-style".
Existing tests should ensure that such a rewrite does not introduce bugs.