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

Issue #408: Fit the susceptible population #904

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

Issue #408: Fit the susceptible population #904

wants to merge 28 commits into from

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Dec 19, 2024

Description

This PR closes #408 by allowing for fitting the susceptible population using the new distribution interface.

Maybe to do:

  • it does not add a inference unit test but we didn't have one before for susceptibility
  • This doesn't address if we should be reporting the susceptibility adjusted or unadjusted Rt.

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.

@seabbs seabbs requested a review from sbfnk December 19, 2024 18:23
@seabbs seabbs enabled auto-merge December 20, 2024 16:49
@seabbs
Copy link
Contributor Author

seabbs commented Dec 20, 2024

Maybe to do:

I am minded not to do any of these unless you feel strongly

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.

This is really nice!

  • This doesn't address if we should be reporting the susceptibility adjusted or unadjusted Rt.

We already have a gen_R in generated quantities (for the nonmechanistic model) - so we could in principle model unadj_R and then always generate R in generated quantities?

R/opts.R Outdated Show resolved Hide resolved
inst/stan/functions/infections.stan Outdated Show resolved Hide resolved
@sbfnk
Copy link
Contributor

sbfnk commented Dec 20, 2024

Maybe to do:

I am minded not to do any of these unless you feel strongly

I don't feel strongly - not quite sure what's best to report anyway. Probably worth a new Issue though as we might be calling stuff R when it's not actually R.

This comment was marked as outdated.

R/opts.R Outdated Show resolved Hide resolved
inst/stan/functions/infections.stan Outdated Show resolved Hide resolved
@seabbs

This comment was marked as outdated.

Copy link
Contributor

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

  • ❗🐌default: 20.1s -> 22.5s [+1.1%, +22.44%]
  • ✔️no_delays: 25.1s -> 23.8s [-15.01%, +4.71%]
  • ✔️random_walk: 9.04s -> 13.3s [-32.12%, +126.71%]
  • ✔️stationary: 11.6s -> 13.2s [-10.22%, +38.66%]
  • ✔️uncertain: 34.9s -> 33.9s [-12.84%, +7.14%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs seabbs requested a review from sbfnk January 10, 2025 16:25
Copy link
Contributor

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

  • ✔️default: 22.1s -> 21s [-19.07%, +8.83%]
  • ✔️no_delays: 25.2s -> 24.6s [-15.87%, +11.05%]
  • ✔️random_walk: 9.17s -> 9.18s [-10.79%, +11.02%]
  • ✔️stationary: 12s -> 13s [-11.45%, +27.35%]
  • ✔️uncertain: 34.5s -> 35.4s [-7.75%, +13.15%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

R/opts.R Outdated Show resolved Hide resolved
R/simulate_infections.R Outdated Show resolved Hide resolved
inst/stan/data/rt.stan Outdated Show resolved Hide resolved
inst/stan/data/simulation_rt.stan Outdated Show resolved Hide resolved
inst/stan/functions/infections.stan Outdated Show resolved Hide resolved
tests/testthat/test-rt_opts.R Outdated Show resolved Hide resolved
inst/stan/functions/infections.stan Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor Author

seabbs commented Jan 23, 2025

So had a bit more of a look at this and on a sight read it looks okay. However when trying to use it to fit models there are serious problems. In the old mode (i.e. forecast) all post data infection estimates are zero and in the all mode it just breaks. Clearly I am missing something so will circle back tomorrow.

Copy link
Contributor

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

  • ✔️default: 21.9s -> 23.4s [-1.81%, +15.39%]
  • ✔️no_delays: 24.2s -> 23s [-15.07%, +5.15%]
  • ✔️random_walk: 9.2s -> 10.1s [-7.67%, +27.49%]
  • ✔️stationary: 12.3s -> 12.7s [-15.6%, +21.63%]
  • ✔️uncertain: 35.4s -> 34.3s [-17.12%, +10.63%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Contributor

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

  • ✔️default: 23.6s -> 41.2s [-34.85%, +184.31%]
  • ✔️no_delays: 25.7s -> 29.4s [-4.2%, +33.01%]
  • ✔️random_walk: 9.17s -> 9.4s [-3.48%, +8.66%]
  • ✔️stationary: 11.8s -> 29.9s [-210.48%, +519.8%]
  • ✔️uncertain: 35s -> 45.8s [-42.9%, +104.43%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 24, 2025

Might the issue be that every parameter gets initialised with a standard normal which in the case of population size is a very long way from any meaningful posterior density (and probably somewhere that the marginal likelihood surface is quite flat)?

@seabbs
Copy link
Contributor Author

seabbs commented Jan 24, 2025

ah yes that could explain the initialisation errors for the all model but not why the infections are all nearly zero in the forecast only model

@sbfnk
Copy link
Contributor

sbfnk commented Jan 24, 2025

ah yes that could explain the initialisation errors for the all model but not why the infections are all nearly zero in the forecast only model

I'm thinking if it's forecast only then the estimate of pop is not informed data so it should converge to the prior - but if it's a long way from the prior and the marginal distribution is fairly flat then it might struggle to get there?

@sbfnk
Copy link
Contributor

sbfnk commented Jan 27, 2025

Also wondering if the use of fmax here is creating issues for the sampler. Is the issue here that this interferes with the early growth calculation if it yields more infections than the population size?

infections[s + uot] = (pop - cum_infections[s]) * fmax(0, 1 - exp_adj_Rt);

@sbfnk
Copy link
Contributor

sbfnk commented Feb 4, 2025

I have tested this as well as a few minor modifications and ran into serious issues. Is there some deep-rooted issue where the likelihood surface becomes really difficult to explore when you're in a situation where depletion of susceptilbles suppresses counts to zero whatever the other parameters?

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.

Allow for fitting the susceptible population
3 participants