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

Remove tidyverse dependencies? #27

Open
edonnachie opened this issue Feb 27, 2023 · 3 comments
Open

Remove tidyverse dependencies? #27

edonnachie opened this issue Feb 27, 2023 · 3 comments

Comments

@edonnachie
Copy link
Owner

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.

@feinmann
Copy link

Native pipe would restrict you to R>=4.1.0.

@edonnachie
Copy link
Owner Author

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.

@edonnachie
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants