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
Feature missing reference support code #138
Conversation
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.
Again, great work, really close now to getting the missingness model running.
If I understand correctly, for the missingness likelihood, we
- still need to integrate the ref_miss_prop
- add a matrix storing the without-reference date obs while going through the snapshots
- do the broadcasting as proposed in Add stan code changes for missing data model #107 for the without-reference date matrix
- transform the matrix into a vector by log-sum-exping over the columns and
- add another call to obs_lmpfs for this second vector
Anything else?
Yes, I think you have got all the changes we need. We also need some postprocessing work to make plotting/summary etc part of the tooling. The only other things I want to add to this PR are:
|
Note that rather than adding tests I instead added more lower level functions. I also used these to update the generated quantities to use the flat data structure. |
Added in 706388c |
Codecov Report
@@ Coverage Diff @@
## develop #138 +/- ##
===========================================
- Coverage 86.20% 86.14% -0.07%
===========================================
Files 12 12
Lines 1196 1234 +38
===========================================
+ Hits 1031 1063 +32
- Misses 165 171 +6
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
I think everything planned for this PR is now in place so pinged for another review. As quite a few A quick summary of what is here (sorry got a little out of hand):
I think as long as correct most of these changes are fairly straightforward. There is an argument we liked the gq being verbose and different from other code as a check on our model but to be honest I prefer unit tests etc and hopefully sharing more code will make future additions easier. Note: Going with a flat structure here poses some potential problems for allocating observations to report dates. One solution would be to pass in a look up for report dates and groups and iterate over it. I think the proposed matrix approach will still work but I may have made it harder. |
Woops! |
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.
Looks all good to me. I added one proposal commit
This PR adds support code for adding modelling of missing reference dates (#43). This includes updating
epinowcast()
to haveenw_missing()
as an argument (it cannot be optional as stan complains about missing data even when it has zero dimensions), updatingenw_missing()
to have a not on mode, adding support for missing effects and priors, adding a new grouped version of the observation likelihood in which we can add likelihood changes from #107, and adding a prototype function for simulating missing data (we should probably look at adding this andenw_incidence_to_cumulative()
to the package but I am aware we are getting lots of exported functions and it may be a bit overwhelming/hard to support in the long term.Alongside #107 this leaves post-processing as a final support step in terms of supporting missing dates to the same level as non-missing dates.
Note the grouped observation likelihood is likely not quite there - tricky keeping track of the indexing. It would be easier if both grouped and snapshot versions could be used without missing data (as should return the same thing) but this might be a bit annoying to support internally.