Public-Health-Scotland / phsmethods

An R package to standardise methods used in Public Health Scotland (https://public-health-scotland.github.io/phsmethods/)
https://public-health-scotland.github.io/phsmethods/
54 stars 13 forks source link

Speed up `chi_check` #80

Closed Moohan closed 1 year ago

Moohan commented 1 year ago

I've been using the chi_check function a lot and it can be quite slow on large lists of CHIs. I did some benchmarking and profiling of the existing function and came up with some optimisations:

I think the first two changes are fairly uncontroversial but the last introduces a C++ function to the package (I used CPP11, over RCPP after reading this article), this means that the package now needs to be complied and requires a few extra bits of infrastructure. This should be fine for users as plenty of tidyverse packages already use the same C++ infrastructure but it does make phsmethods slightly harder to maintain than if it was all written in R.

codecov[bot] commented 1 year ago

Codecov Report

Merging #80 (fb5bf82) into dev (3fb87e5) will increase coverage by 1.15%. The diff coverage is 100.00%.

:exclamation: Current head fb5bf82 differs from pull request most recent head 6dd2f6e. Consider uploading reports for the commit 6dd2f6e to get more accurate results

@@             Coverage Diff             @@
##              dev       #80      +/-   ##
===========================================
+ Coverage   98.84%   100.00%   +1.15%     
===========================================
  Files          12        13       +1     
  Lines         346       343       -3     
===========================================
+ Hits          342       343       +1     
+ Misses          4         0       -4     
Impacted Files Coverage Δ
R/age_calculate.R 100.00% <100.00%> (ø)
R/chi_check.R 100.00% <100.00%> (ø)
src/check_digit.cpp 100.00% <100.00%> (ø)
R/dob_from_chi.R 100.00% <0.00%> (+7.01%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us.

Tina815 commented 1 year ago

We have decided to use R code to improve the speed, so will open a new request for that.