search

epiverse-trace / episoap

[Not published - under active development] A Store of Outbreak Analytics Pipelines Provided as Rmarkdown Report Templates
https://epiverse-trace.github.io/episoap/
Other
4 stars 2 forks source link

Updates transmissibility report lockfiles and deal with incidence2 breaking changes #77

Closed Bisaloo closed 1 year ago

Bisaloo commented 1 year ago

I'm not super happy about the mix between accessor functions and direct subsetting but I don't see any other clear & straightforward way to do this when we deal with objects that lose the incidence class.

Bisaloo commented 1 year ago

Not ran reports but, by visual inspection, the changes seem correct. A couple of points which you are likely aware of (I've no strong opinion here just voicing for awareness).

  1. If you are only looking at a single count variable and grouping variable then direct use of {grates} rather than {incidence2} may be preferable. This depends on end-user of course and there awareness to underlying packages (e.g. are they using as is or tweaking?).

  2. incidence() does allow you to specify the resulting count variable column name as an argument. This would allow you to define relevant columns in variables at the start and then treat everything as if a normal data frame for consistency of access. This is less good when used programatically but potentially better for more interactive use??

Like I say, just flagging for info rather than a strong opinion here.

No strong opinions on this either. I would have no problem making the switch.

For additional context, I believe Thibaut wanted to add a as.incidence2.linelist() method (https://github.com/epiverse-trace/episoap/issues/4#issuecomment-1266905000), which may justify the use of incidence2 over grates. But this hasn't happened and I'm not completely sure if it will happen at some point or not.

Bisaloo commented 1 year ago

Thanks for your comments, Tim! I have opened a new issue (#79) on the topic of the accessors since I'm still undecided but I want to move forward with this PR.