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

Use natural parameters when specifying uncertain distributions #893

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

Conversation

jamesmbaazam
Copy link
Contributor

@jamesmbaazam jamesmbaazam commented Dec 12, 2024

Description

This PR closes #870. I switch all uncertain parameters expressed in using the unnatural parameters to the natural ones and align the accompanying text.

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.

@jamesmbaazam
Copy link
Contributor Author

jamesmbaazam commented Dec 12, 2024

@sbfnk I'm getting this error in the CI. Is this not allowed or do we need to update the checks in check_stan_delays() OR make it clear that if we want to specify a fixed value, it doesn't have to be passed to Fixed()?

library(EpiNow2)
#> 
#> Attaching package: 'EpiNow2'
#> The following object is masked from 'package:stats':
#> 
#>     Gamma
try(
  delay_opts(
    LogNormal(
      meanlog = EpiNow2::Fixed(2),
      sdlog = EpiNow2::Fixed(1),
      max = 10
    )
  )
)
#> Error in check_stan_delay(dist) : 
#>   ! Delay distributions passed to the model need to have parameters that
#>   are either numeric or normally distributed with numeric parameters and
#>   infinite maximum.

Created on 2024-12-12 with reprex v2.1.1

@sbfnk
Copy link
Contributor

sbfnk commented Dec 12, 2024

I think it's as the error says - either has to be numeric or Normal. We could extend this to a Fixed distribution, I don't feel strongly either way.

@seabbs seabbs requested a review from sbfnk December 12, 2024 16:16
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

The original issue only suggested doing this for parameterisations with uncertainty to avoid the warning. For fixed parameters there is no issue - perhaps keep these as they are?

There are a few instances of examples not yet captured here, e.g. various in dist_spec.r and for delay_opts() and the vignettes. I think a full-text search for "mean = Normal" will get most of them.

NEWS.md Outdated Show resolved Hide resolved
R/epinow.R Outdated Show resolved Hide resolved
R/estimate_infections.R Outdated Show resolved Hide resolved
R/regional_epinow.R Outdated Show resolved Hide resolved
R/simulate_infections.R Outdated Show resolved Hide resolved
R/epinow.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@jamesmbaazam jamesmbaazam changed the title Use natural parameters in all examples Use natural parameters when specifying uncertain distributions Dec 13, 2024
@jamesmbaazam jamesmbaazam requested a review from sbfnk December 13, 2024 16:43
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.

Update examples that use non-natural parameterisation and uncertain parameters
2 participants