-
Notifications
You must be signed in to change notification settings - Fork 4
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
2024-08-08 update : adding spatial to wwinference model. #56
2024-08-08 update : adding spatial to wwinference model. #56
Conversation
This is not ready yet, but I wanted to take input from both of you : @dylanhmorris and @kaitejohnson . Spatial components are in the model and using the testfile, I ran everything and things definitely change. There is a lot more variability, mainly in the forecast period. Rt take values that reach past 2, and site Rt's occasionally reach past 5. |
Interesting! How does that compare to the ground truth R(t) values in each subpopulation? Might be worth making a plot of the estimated R(t)s and the known R(t)s that you simulated. Also, how tight are your priors around the ground truth values of |
Yes, I worked on changing these values and how big they are, but it seems like the thing that makes these values explode is using I could have written the generalized variance into stan wrong, it might be good to look over that particular part @kaitejohnson and @dylanhmorris on lines 236 in the wwinference.stan model. |
@cbernalz can you make some plots and share them in the other repo? |
You could share them here because it only simulated data? |
@dylanhmorris and @kaitejohnson I am currently adjusting the |
Yep, I think that is the idea! So if |
Great, @kaitejohnson and @dylanhmorris I figured out the problem was the strict prior placed on |
Weirdly I am not able to see the full set of CI jobs (including the failure) for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor suggestions / comments, @cbernalz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbernalz This looks really really good! Great job putting this all together, and apologies for letting this PR get so big!
My only high level suggestion that I think requires fixing in this PR is the comment about not putting the distance matrix in as default to the wwinference()
wrapper function. And requiring the user specify that will require editing the vignette but it should be a pretty easy fix. The other stuff like moving the distance matrix to be package data as well can be done in another PR! I just want to make sure it's super easy for us to modify the distance matrix that we either pass in as a default to generate_simulated_data()
or that we fit the model to in a simulation analysis.
@dylanhmorris and @kaitejohnson I updated all of your comments and the biggest change is that I added a way to use either an independent correlation function (default if no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, sounds like you've also pretty much figured out how to do the switch on and off too which is great. See my one comment about how to specify that from the "user" side in the wwinference()
function. But think thats for another PR.
Great work on this!! It's looking really nice. Excited to see it in action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good @cbernalz. A few minor suggestions / fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @cbernalz
A couple minor things I missed before about explicitly noting 1
and 0
as integers rather than floats (1L
, 0L
). Can take or leave, and then merge.
Co-authored-by: Dylan H. Morris <[email protected]>
Co-authored-by: Dylan H. Morris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will let you merge, @cbernalz
This PR is regarding the following issue : #44 (comment) . The model that will be added into this will be coming from this issue : #25 (comment) .