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

Change all priors to use <dist_spec> #871

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

Change all priors to use <dist_spec> #871

wants to merge 10 commits into from

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Nov 26, 2024

Description

This PR closes #525 by changing all parameters to use the new interface.

In doing so and in order to keep this manageable the internals of handling parameters had to be updated a fair bit. This is now done similarly to delays in parameter arrays rather than named parameters. In principle this should make it easier to extend the model later with more parameters / probability distributions / time-varying parameters etc.

Things that could be added later fairly easy if deemed useful:

  1. switch delay distribution parameters to use the new stan setup for parameters (this might simplify code so probably a good idea)
  2. allow uncertain parameters of prior distributions other than delays

From tests it seems functionality has been preserved but it would be good for people to check if this is actually an improvement.

One thing to note is that now all parameters are initialised from (truncated) standard normal distributions so will have to monitor if this leads to any issues.

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.

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.

Wow - this is a lot. On the face of it looks good but going to need some time to digest

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if deedb9e is merged into main:

  • ✔️default: 24.1s -> 20.8s [-28.62%, +0.99%]
  • ✔️no_delays: 26.2s -> 26.2s [-10.42%, +10.39%]
  • ✔️random_walk: 9.66s -> 9.62s [-7.59%, +6.74%]
  • 🚀stationary: 14.3s -> 11.8s [-30.72%, -3.89%]
  • ✔️uncertain: 37.4s -> 36.8s [-13.63%, +10.13%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

it won't work anyway as the R code has changed too much
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 333de46 is merged into main:

  • ✔️default: 22.4s -> 21.7s [-16.65%, +10.78%]
  • ✔️no_delays: 27s -> 27.8s [-11.93%, +18.38%]
  • ✔️random_walk: 9.69s -> 9.99s [-2.32%, +8.41%]
  • 🚀stationary: 16.1s -> 12.5s [-43.91%, -0.72%]
  • ✔️uncertain: 39.8s -> 38.4s [-14.72%, +8.04%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor Author

sbfnk commented Nov 27, 2024

One thing to note is that the lengthscale prior is currently not included in this, because of the extra logic in


It might be worth deciding on a single shape of the prior and then this could also be specified as a <dist_spec> (though this currently doesn't support specifying a minimum, which would either have to be dropped or functionality added).

@sbfnk
Copy link
Contributor Author

sbfnk commented Nov 27, 2024

Related to that merging this would enable a more comprehensive exploration of parameter choices and identifiability than what's done in #855 (not requiring editing the stan model) as we can fix parameters and/or try out different distributions with different parameters.

@seabbs
Copy link
Contributor

seabbs commented Nov 27, 2024

which would either have to be dropped or functionality added

I think we would need to add this as from memory it really isn't identifiable without setting some bounds

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.

Change all priors to new dist interface, not just delays
2 participants