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.

Sign up for GitHub

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

Issue 793: Improve documentation for those specifying a non-parametric delay #799

Merged
merged 8 commits into from
Oct 1, 2024
Merged

Issue 793: Improve documentation for those specifying a non-parametric delay #799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2177920
show example of non-parametric specification of GI and delay
kaitejohnson Sep 29, 2024
75f663c
remove comments on indexing in code
kaitejohnson Sep 29, 2024
2b29030
update vignette to explain GI should be zero indexed with first eleme…
kaitejohnson Oct 1, 2024
d6df0b3
Merge branch 'main' into main
kaitejohnson Oct 1, 2024
376835e
Update vignettes/EpiNow2.Rmd
kaitejohnson Oct 1, 2024
3c0d6ae
move changes to Rmd.orig
kaitejohnson Oct 1, 2024
87a8234
add to DESCRIPTION
kaitejohnson Oct 1, 2024
279914e
Merge branch 'main' into main
kaitejohnson Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ Authors@R:
family = "Johnson",
role = "ctb",
email = "[email protected]"),
person(given = "Kaitlyn",
family = "Johnson",
role = "ctb",
email = "[email protected]",
comment = c(ORCID = "0000-0001-8011-0012")),
person(given = "EpiForecasts",
role = "aut"),
person(given = "Sebastian",
Expand Down
17 changes: 15 additions & 2 deletions vignettes/EpiNow2.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ library(EpiNow2)

### Reporting delays, incubation period and generation time

Distributions can be supplied in two ways. First, one can supplying delay data to `estimate_delay()`, where a subsampled bootstrapped lognormal will be fit to account for uncertainty in the observed data without being biased by changes in incidence (see `?EpiNow2::estimate_delay()`).
Distributions can be supplied in two ways. First, one can supply delay data to `estimate_delay()`, where a subsampled bootstrapped lognormal will be fit to account for uncertainty in the observed data without being biased by changes in incidence (see `?EpiNow2::estimate_delay()`).

Second, one can specify predetermined delays with uncertainty using the distribution functions such as `Gamma` or `Lognormal`. An arbitrary number of delay distributions are supported in `dist_spec()` with a common use case being an incubation period followed by a reporting delay. For more information on specifying distributions see (see `?EpiNow2::Distributions`).

Expand Down Expand Up @@ -87,6 +87,20 @@ example_incubation_period
#> 0.069
```

Users can also pass a non-parametric delay distribution vector using the `NonParametric` option
for both the generation interval and reporting delays. It is important to note that if doing so,
both delay distributions are 0-indexed, meaning the first element corresponds to the probability mass
at day 0 of an individual's infection. Because the discretised renewal equation doesn't support mass on day 0, the generation interval should be passed in as a 0-indexed vector with a mass of zero on day 0.
If this is not the case, a warning will indicate that the vector is being left-truncated and renormalized.

```r
example_non_parametric_gi <- NonParametric(pmf = c(0, 0.3, 0.5, 0.2))
Copy link
Contributor

@jamesmbaazam jamesmbaazam Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I know this has been merged but I was catching up and noticed the following: given the preceding text before the chunk, I would have expected to see the warning showcased here, i.e., the example should use gt_opts() instead of the direct call to NonParametric() like so

> gt_opts(NonParametric(pmf = c(0.1, 0.3, 0.5, 0.2)))
- nonparametric distribution
  PMF: [0.091 0.27 0.45 0.18]
Warning message:
Specifying nonparametric generation times with nonzero first element was deprecated in
EpiNow2 1.6.0.
ℹ Since zero generation times are not supported by the model, the generation time will be
  left-truncated at one.
ℹ In future versions this will cause an error. Please ensure that the first element of
  the nonparametric generation interval is zero.
ℹ The deprecated feature was likely used in the EpiNow2 package.
  Please report the issue at <https://github.com/epiforecasts/EpiNow2/issues>.
This warning is displayed once every 8 hours.
Call `lifecycle::last_lifecycle_warnings()` to see where this warning was generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamesmbaazam Do you think it makes sense to include a non-parametric pmf that produces this warning as you have shown?

The one currently included has a value of 0 on day 0, so it won't produce the warning! Perhaps the preceding text isn't clear enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the text is fine but personally would have expected the code sample to showcase the warning thrown when the delay is not 0-indexed. Additionally, I would suggest to explicitly print the results of example_non_parametric_gi and example_non_parametric_delay in the vignette.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can open an issue and subsequent PR to add this.

Do you think there should be an example for both the correct specification (so with the 0 on day 0) and the incorrect specification that results in a warning (as you demonstrated)? My only concern is about bloating the vignette, otherwise I think it could make sense to show both.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced we want to show a way of specifying that we're discouraging in the warning (and that will cause an error in the future). Perhaps we should just remove the highlighted sentence if it's confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbfnk You're right about not encouraging the wrong specification.

Maybe, we can reword this part "If this is not the case, a warning will indicate that the vector is being left-truncated and normalized." -> "If this is not the case, the vector will be left-truncated and normalized."

I do see that the doc of gt_opts() has the following wording: "Because the discretised renewal equation used in the package does not support zero generation times, any distribution specified here will be left-truncated at one, i.e. the first element of the nonparametric or discretised probability distribution used for the generation time is set to zero and the resulting distribution renormalised." Shouldn't we just reuse that here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that in the vignette we should only tell users how to specify this correctly and not what happens if they fail to do so (which they'll find out with the warning anyway). So I'd vote for removing the sentence altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaitejohnson Would you like to take this one 😃 ?


example_non_parametric_delay <- NonParametric(pmf = c(0.01, 0.1, 0.5, 0.3, 0.09))
```
These distributions are passed to downstream functions in the same way that the
parametric distributions are.

Now, to the functions.

### [epinow()](https://epiforecasts.io/EpiNow2/reference/epinow.html)
Expand Down Expand Up @@ -129,7 +143,6 @@ names(estimates)

Both summary measures and posterior samples are returned for all parameters in an easily explored format which can be accessed using `summary`. The default is to return a summary table of estimates for key parameters at the latest date partially supported by data.


```r
knitr::kable(summary(estimates))
```
Expand Down
16 changes: 15 additions & 1 deletion vignettes/EpiNow2.Rmd.orig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ library(EpiNow2)

### Reporting delays, incubation period and generation time

Distributions can be supplied in two ways. First, one can supplying delay data to `estimate_delay()`, where a subsampled bootstrapped lognormal will be fit to account for uncertainty in the observed data without being biased by changes in incidence (see `?EpiNow2::estimate_delay()`).
Distributions can be supplied in two ways. First, one can supply delay data to `estimate_delay()`, where a subsampled bootstrapped lognormal will be fit to account for uncertainty in the observed data without being biased by changes in incidence (see `?EpiNow2::estimate_delay()`).

Second, one can specify predetermined delays with uncertainty using the distribution functions such as `Gamma` or `Lognormal`. An arbitrary number of delay distributions are supported in `dist_spec()` with a common use case being an incubation period followed by a reporting delay. For more information on specifying distributions see (see `?EpiNow2::Distributions`).

Expand All @@ -61,6 +61,20 @@ example_generation_time
example_incubation_period
```

Users can also pass a non-parametric delay distribution vector using the `NonParametric` option
for both the generation interval and reporting delays. It is important to note that if doing so,
both delay distributions are 0-indexed, meaning the first element corresponds to the probability mass
at day 0 of an individual's infection. Because the discretised renewal equation doesn't support mass on day 0, the generation interval should be passed in as a 0-indexed vector with a mass of zero on day 0.
If this is not the case, a warning will indicate that the vector is being left-truncated and renormalized.

```{r}
example_non_parametric_gi <- NonParametric(pmf = c(0, 0.3, 0.5, 0.2))

example_non_parametric_delay <- NonParametric(pmf = c(0.01, 0.1, 0.5, 0.3, 0.09))
```
These distributions are passed to downstream functions in the same way that the
parametric distributions are.

Now, to the functions.

### [epinow()](https://epiforecasts.io/EpiNow2/reference/epinow.html)
Expand Down
Loading