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

Add check and warning for PMFs longer than the data #998

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jamesmbaazam
Copy link
Contributor

Description

This PR closes #739.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

delays <- list(...)
flat_delays <- do.call(c, delays)
# Track which component each delay came from
delay_names <- rep(names(delays), vapply(delays, EpiNow2:::ndist, numeric(1)))
Copy link
Contributor

Choose a reason for hiding this comment

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

you dont need ::: for internal functions. This is throwing a valid linting issue

Copy link
Contributor

@seabbs seabbs 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. The vast majority of the linting issues being thrown are from the linting change vs this PR so don't need to be addressed here.

My main comment is do we just need to check individual lengths or do we also need to check the lengths of combined delays (i.e. the reporting delay when it is a reporting delay and a incubation period)?


# Check lengths and collect info about exceeding PMFs
pmf_longer_than_data <- vapply(flat_delays[np_delays], function(x) {
length(x$pmf) > nrow(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to also check if the total reporting delay is longer than the data or the case where two distributions are passed to the generation time (not sure this is something you can actually do?)

@jamesmbaazam jamesmbaazam changed the title Add check for PMFs longer than the data Add check and warning for PMFs longer than the data Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a warning when the length of delays are longer than the supplied data
2 participants