Update <epiparameter> constructor function and internal refactor
#381
Conversation
…stribution_params
…params and update usage
…all convert_summary_stats_to_params internally
…eate_prob_distribution
… can be done in epiparameter() before calling new_epiparameter
…with prob_distribution
…e_prob_distribution as default for prob_distribution arg
…tion object in epiparameter()
…() and make checks stricter
…w_epiparameter call in epiparameter()
…tion instead of prob_dist
…n to create_prob_distribution
…in conversion and list uncertainty
… is created for <epiparameter>
There was a problem hiding this comment.
I think this change is going in the right direction. In particular, I see how it opens up the potential for more interoperability with other ways of defining probability distributions. We could for example imagine at some point accepting distributions from EpiNow2 in prob_distribution, which was more difficult with the previous structure.
On the topic of renaming arguments, it would be very helpful in the future if you can do it in a single commit. This way, it's really possible to review while ignoring the non-functional changes.
| #' ) | ||
| is_epiparameter_params <- function(prob_dist, prob_dist_params) { | ||
| if (is.na(prob_dist) || anyNA(prob_dist_params)) { | ||
| is_epiparameter_params <- function(prob_distribution, |
There was a problem hiding this comment.
My intuition would have been to have this as an internal helper. I've been quite removed from the codebase and may likely miss some subtleties. Happy for you to make the final call but please take a moment to consider if it should really be exported.
There was a problem hiding this comment.
The reason I exported it was I thought it might be useful to some users to check if the parameters they want to input are valid for parameterising an <epiparameter> as not every possible parameterisation is supported.
If you think this should be made internal please let me know and I'll unexport it.
| # return prob_dist object | ||
| prob_dist | ||
| # return prob_distribution object | ||
| prob_distribution |
There was a problem hiding this comment.
Should this have a class (only used internally)? It may ultimately allow us to get objects from other origins in epiparameter() call.
There was a problem hiding this comment.
Wrapping the probability distribution object in a subclass? We could do this, but it might have complications for <distribution> objects from {distributional} as its built on {vctrs} which is a OOP framework I'm less familiar with.
Please open an issue stating the benefits of giving this object a class and I can implement in a follow-up PR.
| meanlog: 0.660 | ||
| sdlog: 1.205 | ||
| meanlog: 1.386 | ||
| sdlog: 0.593 |
There was a problem hiding this comment.
These are changing due to a bug fix in #378 which are now updating the automatically calculated parameters of these database entries, so this change is expected.
|
Thanks for putting together, these changes look sensible, especially if open up more flexibility. Quick question - does the %||% operator involve a new dependency? |
|
Thanks both for your comments. @adamkucharski |
|
This PR is failing the tests workflow on GHA due to a change in {epireview} which is loade in the |
This is a relatively large PR with several aspects of refactoring. The aim of this sequence of developments is to simplify the code base of the package, to make the construction of
<epiparameter>objects more modular and remove ambiguity in function arguments.This PR contains Breaking changes.
The
epiparameter()function has a new function signature where the arguments for constructing a probability distribution object have been removed and moved to thecreate_prob_distribution()function. This was previously namedcreate_prob_dist()but has been renamed as have several arguments that usedprob_distto remove any ambiguity to_dist(as requested by a package user).The new design of
epiparameter()andcreate_prob_distribution()have helped greatly simplynew_epiparameter().The handling of uncertainty when constructing
<epiparameter>objects has been enhanced to now work when probability distribution objects are constructed (assumingauto_calc_params = TRUE) to match the parameters, whereas previously this would have caused a mismatch between the parameters and the parameter uncertainty.The
as_epiparameter()method for converting from {epireview} tables has been updated to create parameterised<epiparameter>objects when adequate summary statistics are provided and a user-specified probability distribution is supplied via....The
prob_dist_paramsargument has been removed from.calc_dist_params()thanks to changes made in #379. It also calls theconvert_summary_stats_to_params()function for converting mean and dispersion parameters of lognormal distribution, fixing a likely bug in the previous conversion implemented in.calc_dist_params().This PR adds the
%||%operator to the package.Note: many of the diffs in this PR are changing
prob_disttoprob_distributioninside functions. This is a non-functional change. Many of these can be skimmed as they should not make any functional change to the package. Although spotting instances where I have missed someprob_distor updated them incorrectly would of course be appreciated.Unit tests, snapshots and documentation have been updated where necessary.