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

Expose dt/steps_per_day control #188

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Expose dt/steps_per_day control #188

wants to merge 7 commits into from

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Jan 8, 2021

Just the core parameter handling part done here. Someone needs to check that the epi looks ok.

We should expand the testing to cover things like Rt but I wonder if this might actually not be that bad

Fixes #187

@richfitz richfitz marked this pull request as draft January 8, 2021 17:57
@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #188 (2de2a6b) into master (56c879b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #188   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1196      1200    +4     
=========================================
+ Hits          1196      1200    +4     
Impacted Files Coverage Δ
R/carehomes.R 100.00% <100.00%> (ø)
R/parameters.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56c879b...9634e39. Read the comment docs.

@annecori
Copy link
Contributor

annecori commented Jan 19, 2021

I added some tests but the Rt test is failing:

test_that("Can compute Rt with larger timestep", {

Maybe @edknock you could look at this?
I now have the same model with dt = 1, 1/4 and 1/100 and the Rt are all different, although not to the extent that it could just be a forgotten *dt or /dt somewhere. Still some noticeable differences (5-15%) which require some investigation...

@annecori
Copy link
Contributor

Update on the above. I implemented a function to update the gamma parameters before running the model, so that the resulting mean duration of the compartments using the discretised gamma would match the mean of the continuous gamma distribution. This has fixed the Rt calculations - which are now similar across different dt values, but generates substantially different epidemic dynamics - e.g. attack rate is very different with different rt values.
I think this means we need to rethink the calculation of Rt - I'm not sure it is correct to calculate the Rt using the discretised mean duration - maybe we should just use the mean of the continuous distribution? @edknock would be great if you can have a think about this.

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.

Expose dt, and confirm working if this is changed
2 participants