-
Notifications
You must be signed in to change notification settings - Fork 11
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
New package architecture #85
Conversation
…a and added suggested packages
… and skipped tests to pass check ready for WIP refactor
… input in epidist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joshwlambert, looks good to me overall, and happy to approve. There are a few open conversations though, which seem to be optional suggestions, and I would recommend converting them to new issues to be addressed in a different PR.
Putting down a few things I noticed which might be good to convert to issues or keep in mind for future fixes:
- The vignette chunks are set to
eval = FALSE
since the functions are out of date with the current PR - something to keep in mind. - There are over 1,300 tests which is potentially unwieldy - suggest eventually compressing tests - for example, a single key-value pair comparison (via a snapshot) for the column types in the parameter database, rather than a single
expect
for each column. - L.209 in
epiparam_utils.R
has a hardcoded number of columns. - Might be good to revisit the function
get_percentiles()
- haven't tested it but getting names and converting to decimal values, as well as determining the upper and lower percentile values seems a little hacky at the moment - am I right in thinking one value has to be > 50 and one < 50? An understandable choice, but not mentioned in the documentation. These lines in the tests suggests this is the case:epiparameter/tests/testthat/test-get_percentiles.R
Lines 8 to 9 in 13c4f40
res <- get_percentiles(c(q_05 = 1, q_45 = 10)) expect_identical(res, NA)
This pull request contains a large number of changes influencing the handling of epidemiological data, the API of the
epidist
class and its methods, as well as work on numerous helper functions.Changes to the
{epiparameter}
library:Changes to the epidist class:
{epiparameter}
library. Instead, it is a stand alone class that can be constructed by the user.epidist()
function of reading directly from the library, theepidist_db()
function has been added.epidist
class have been replaced with class methods. These have new names that follow from the naming convention of the{distributional}
package (which is depended on to handle distributions). Update epidist class #73epidist
class can handle continuous (with right-truncation Add right-truncation option for continuous distributions #82) and discretised distributions (using{distributional}
or{distcrete}
). Allow discrete distributions using {distcrete} #81, Add pdf, cdf, quantiles and random sampling methods for epidist class #83epidist
class now follows the structure of having a constructor, helper and validator function.Addition of an
epiparam
class:epidist()
,list_distributions()
andpathogen_summary()
), but there was no way to handle the library and manipulate it in custom ways. Theepiparam
class is a data structure for the library and is a subclass of adata.frame
to provide printing and subsetting methods. Add epiparam class #74epiparam
class follows the structure of having a constructor, helper and validator function.Addition of a
vb_epidist
class:epidist
class for vector-borne diseases in which there is an intrinsic and extrinsic delay (e.g. incubation period) thevb_epidist
class provides a consistent interface by wrapping around theepidist
class. Accomodate vector-borne diseases in database #50vb_epidist
class has the same probability distribution methods as theepidist
class.vb_epidist
class follows the structure of having a constructor, helper and validator function.Addition of JSON validation on library:
Addition of conversion functions:
There are class conversion functions (
as_epidist()
andas_epiparam()
). There is also abind_epiparam()
function that allows all classes defined in the{epiparameter}
package to be bound to an existingepiparam
object.Helper functions for constructing objects have been added to aid in supplying information manually when constructing an
epidist
object. For examplecreate_epidist_citation()
andcreate_epidist_metadata()
.Two exported functions,
calc_dist_params()
andcreate_prob_dist()
are added which are mainly used internally but are exported in case users may find them helpful.Added data protocol vignette that describes the process of building the
{epiparameter}
library. #68The
pathogen_summary()
function has been removed as it's functionality was no longer deemed necessary.Documentation and testing have been updated in line with the above-mentioned developments.