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

Full package review for v0.2.0 release #161

Merged
merged 36 commits into from May 9, 2023
Merged

Full package review for v0.2.0 release #161

merged 36 commits into from May 9, 2023

Conversation

pratikunterwegs
Copy link
Member

@pratikunterwegs pratikunterwegs commented Apr 19, 2023

This PR trials a full repository review for the package, and is also working towards items listed in #163.

@pratikunterwegs pratikunterwegs marked this pull request as ready for review April 19, 2023 14:49
@pratikunterwegs pratikunterwegs self-assigned this Apr 19, 2023
@Bisaloo Bisaloo self-requested a review April 19, 2023 17:53
@pratikunterwegs
Copy link
Member Author

I've just added branch protection rules for the empty branch so that it is read-only, and so that changes are not merged into it by accident.

@pratikunterwegs pratikunterwegs changed the title Full package review Full package review for v0.2.0 release Apr 21, 2023
.github/workflows/Cpp-lint-check.yaml Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/r_eff.r Outdated Show resolved Hide resolved
R/final_size.R Outdated Show resolved Hide resolved
R/final_size.R Outdated Show resolved Hide resolved
inst/include/iterative_solver.h Outdated Show resolved Hide resolved
zeros.fill(0);

Eigen::ArrayXd epi_final_size(nDim); // prev in settings struct
epi_final_size.fill(0.5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a better initial guess thanks to the theory in this domain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. For example, the simple case of finding the final size is included as a function in the tests here as

upper_limit <- function(r0) {
f <- function(par) {
abs(1 - exp(-r0 * par[1]) - par[1])
}
opt <- optim(
par = 0.5, fn = f,
lower = 0.0, upper = 1.0,
method = "Brent"
)
opt
}

The method here also starts with a guess of 0.5 if I've understood correctly, so this might be a standard starting point. Happy for @BlackEdder or @adamkucharski to weigh in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can get a better starting point based on pre-filled estimates from a non-age structured final size equation. If you solve for R0s in 1:20 (for example) that will give you a place to start for all age groups.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I haven't really understood that. We are already passing $R_0$ to the solver. Also the snippet I've linked isn't used in the solver code, it's only in the tests - Hugo is referring to the solver code, where this function isn't available (R vs C++).

I could add an 'initial guess' parameter to the solvers, so that it's 0.5 by default, but could also be a vector coming from an earlier solver run on non-age structured populations? This might get a little recursive.

R/final_size.R Outdated Show resolved Hide resolved
inst/include/iterative_solver.h Outdated Show resolved Hide resolved
inst/include/newton_solver.h Outdated Show resolved Hide resolved
@pratikunterwegs
Copy link
Member Author

Thanks for taking a look @Bisaloo. I've done a little bit of reorganisation to the solver files to move the contact matrix scaling to the main function using a switch(); the solver code now does not use the zeros and copied contact_matrix_-es. I accepted some suggestions for the error function f() in the iterative solver but have moved it away from std::move semantics - see explanation in the comments.

@rozeggo rozeggo requested a review from bahadzie May 3, 2023 08:44
NAMESPACE Outdated Show resolved Hide resolved
Copy link
Member

@bahadzie bahadzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are profiling and/or benchamarking tools used to determine when code needs to be written in C. If so are there any guidelines on when it would be a good idea to use C instead of R.

@pratikunterwegs
Copy link
Member Author

Are profiling and/or benchamarking tools used to determine when code needs to be written in C. If so are there any guidelines on when it would be a good idea to use C instead of R.

This issue has been raised more broadly in epiverse-trace/blueprints#11, and I think the consensus reached in the Slack discussions at least is to use R unless:

  1. There is substantial pre-existing code in another language, which it would easier to use or adapt;
  2. Speed improvements are achievable and important to users.

Re: 1, the current solver algorithms and overall methods were taken from C++ code written by @BlackEdder; and the R code was my translation of those methods; see PRs #4, #12 etc.
Re: 2, we benchmarked (but not profile) both versions, and the Rcpp versions are slightly faster, but not noticeably so to users who are running the function only once. The difference might be for users who run the function many thousands of times - Edwin has previously indicated that this was among his use cases.
So overall, we decided to go with the Rcpp implementation in PR #73.

The considerations around {epidemics} were similar, with the expectation being that models that could be included in the library would have been written in C++ already (see e.g. https://github.com/dchodge/rsv_trans_model or {fluEvidenceSynthesis}; but see {vacamole} as a counterpoint). We have not yet incorporated any external models into {epidemics} so it might be too early to say how this calculation will work out.

Some benchmarks for {finalsize} from the Slack channel here run by @TimTaylor.

#> Unit: microseconds
#>                  expr      min        lq       mean    median        uq       max neval
#>    iterative_solver_r  172.440  187.0590  198.07013  197.3165  205.9345   293.006  1000
#>       newton_solver_r 1614.130 1647.3455 1729.58618 1664.4795 1685.0625 11410.673  1000
#>  iterative_solver_cpp    6.815    9.6935   12.64198   12.8000   15.0175    31.875  1000
#>     newton_solver_cpp  426.709  440.0405  451.06848  450.8010  458.3565   525.540  1000

@pratikunterwegs
Copy link
Member Author

This PR is now moving to tackling issues in #163 - further comments will relate to items in the release checklist.

@pratikunterwegs
Copy link
Member Author

pratikunterwegs commented May 4, 2023

Progress on task items in the release checklist in issue #163 was being tracked in a comment here, and has now been moved to the comments of #163 - see #163 (comment).

@pratikunterwegs pratikunterwegs mentioned this pull request May 4, 2023
26 tasks
@bahadzie bahadzie requested a review from Bisaloo May 4, 2023 11:58
@pratikunterwegs pratikunterwegs changed the base branch from empty to main May 9, 2023 09:51
pratikunterwegs and others added 12 commits May 9, 2023 10:51
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
@pratikunterwegs
Copy link
Member Author

Thank you @Bisaloo @bahadzie and @rozeggo for taking a look at this full package review. The PR target has now changed from empty to main. Once checks pass, I will rebase and merge the PR, and proceed with releasing main to CRAN if that seems alright.

@pratikunterwegs pratikunterwegs merged commit 3bdf06c into main May 9, 2023
13 checks passed
@pratikunterwegs pratikunterwegs deleted the review branch May 9, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants