Use linelist data source in transmissibility report #137
Conversation
There was a problem hiding this comment.
Thanks for these changes.
In an ideal scenario there would be a more flexible interface to allow users to use either line list or aggregated data, but this change resolves #89 so I think we should move forward with it.
I've compiled the template with the new changes on this branch and everything seems to render correctly.
Hi Hugo thanks for this, I'm going to look into it this afternoon |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
=======================================
Coverage 73.07% 73.07%
=======================================
Files 4 4
Lines 26 26
=======================================
Hits 19 19
Misses 7 7 ☔ View full report in Codecov by Sentry. |
Bisaloo
force-pushed
the
use-linelist-data
branch
from
5a7fab4 to
bc880aa
Compare
inst/rmarkdown/templates/transmissibility/skeleton/skeleton.Rmd
Outdated
Show resolved
Hide resolved
| @@ -304,14 +302,15 @@ Here we identify the key data needed in the analyses, including: | |||
| ```{r} | |||
| date_var <- "date" | |||
| group_var <- "region" | |||
| count_var <- "n" | |||
| # Leave count_var as NULL if the data is really a linelist / patient-level data. | |||
There was a problem hiding this comment.
This will have to be included in the documentation for users as well
There was a problem hiding this comment.
Aren't the comments in the report the documentation for users?
There was a problem hiding this comment.
I mean in vignettes as well that explain in more detail how to interact with the templates
There was a problem hiding this comment.
Got it! As far I know though, we don't really have this kind of info in the vignette at the moment. Do you think it can wait for a later PR on this specifically? Or do you believe it has to be part of this PR?
There was a problem hiding this comment.
I was planning to write it up separately in another PR as part of the updates to the package- is that appropriate?
There was a problem hiding this comment.
Yes, sounds great to me. Let's merge this PR then.
inst/rmarkdown/templates/transmissibility/skeleton/skeleton.Rmd
Outdated
Show resolved
Hide resolved
Co-authored-by: CarmenTamayo <[email protected]>
inst/rmarkdown/templates/transmissibility/skeleton/skeleton.Rmd
Outdated
Show resolved
Hide resolved
To clarify we're talking about the generic data format, and not the package
| @@ -304,14 +302,15 @@ Here we identify the key data needed in the analyses, including: | |||
| ```{r} | |||
| date_var <- "date" | |||
| group_var <- "region" | |||
| count_var <- "n" | |||
| # Leave count_var as NULL if the data is really a linelist / patient-level data. | |||
There was a problem hiding this comment.
I was planning to write it up separately in another PR as part of the updates to the package- is that appropriate?
Fix #89
It's slightly annoying and not ideal that the code changes like this between the case when we use individual level data vs aggregated data.
This means users may now wonder how to use aggregated data 😩
Tagging @joshwlambert for review as you opened the original issue.
Tagging @CarmenTamayo for review as package author.