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

Update epidist_db() to give more fine-grained control #160

Merged
merged 17 commits into from
Aug 24, 2023
Merged

Conversation

joshwlambert
Copy link
Member

@joshwlambert joshwlambert commented Aug 18, 2023

This PR addresses #163 (and #91 which was superseded by #163).

The new implementation uses subset argument to allow the use to define subsetting criteria to select the entry from the database they want. It also includes a single_epidist argument to allow uses to specify if they only want a single <epidist> returned.

It also changes is_parameterised() to make it an S3 generic which has methods for <epiparam> and <epidist> objects.

@joshwlambert joshwlambert added the enhancement New feature or request label Aug 18, 2023
@pratikunterwegs
Copy link
Collaborator

Thanks - just flagging that the new implementation would lead to this code

epidist_db(
  disease = "COVID-19",
  epi_dist = "onset_to_death",
  author = "Linton_etal"
)

returning multiple <epidists> after this change, rather than one, if I'm correct. Will file an issue in {cfr} once this PR is merged, as this would require some code in vignettes and possibly examples to be updated to either use single_epidist or manually subset later.

Copy link
Collaborator

@pratikunterwegs pratikunterwegs left a comment

Choose a reason for hiding this comment

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

Looks good to me - discussion continues on Slack so I'd be happy to take another look once the PR stabilises.

Copy link
Collaborator

@pratikunterwegs pratikunterwegs left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @joshwlambert - looks okay to me other than some minor points flagged here.

R/checkers.R Show resolved Hide resolved
R/epidist.R Outdated Show resolved Hide resolved
R/epidist_db.R Show resolved Hide resolved
R/epidist_db.R Outdated Show resolved Hide resolved
tests/testthat/test-checkers.R Outdated Show resolved Hide resolved
tests/testthat/test-checkers.R Outdated Show resolved Hide resolved
tests/testthat/test-epidist_db.R Show resolved Hide resolved
tests/testthat/test-epidist_db.R Outdated Show resolved Hide resolved
tests/testthat/test-epidist_db.R Show resolved Hide resolved
Copy link
Collaborator

@pratikunterwegs pratikunterwegs left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for making the changes. Just the one issue of unused eparam in test-checkers.R, but otherwise feel free to merge.

@joshwlambert joshwlambert merged commit ac68ec5 into main Aug 24, 2023
9 checks passed
@joshwlambert joshwlambert deleted the db_hier branch August 24, 2023 10:25
#'
#' @return A boolean logical for <epidist> or vector of boolean for each entry
#' in the <epiparam>.
#' @export
Copy link
Member

Choose a reason for hiding this comment

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

Since this function is exported, would it be possible to add in the description or in the examples a typical use case for users? At the moment, I can only think about internal use cases.

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

Successfully merging this pull request may close these issues.

3 participants