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

Distributions for parameters from {epiparameter} vs manual input in transmissibility report #109

Open CarmenTamayo opened 6 months ago

CarmenTamayo commented 6 months ago

On PR #100 , {distcrete} and {epitrix} have been replaced by {epiparameter}, to discretise distributions and convert summary statistics into distribution parameters, respectively.

Now the issue is that epiparameter::discrete requires an epidist object, which is currently not generated when users provide parameters manually, rather than through the epiparameter database. This also affects the syntax of the plotting function, which is different when using an epidist object too.

The solution would be either to revert to using {distcrete} or to convert the manually added parameters and their distribution to an epidist object.

thoughts? @Bisaloo

Bisaloo commented 6 months ago

I'm not sure.

Discretizing has side effects I didn't anticipate when this code was written. I thought as distcrete as just a nice interface and didn't realize it was actually an approximation of the continuous distribution.

As the same time, epidist objects at the moment require that you pass a disease name which we don't necessarily know. I don't know if it's an issue or if it would be okay to pass "Disease X".

CarmenTamayo commented 6 months ago

Could we add the disease name to the params list? as it is for epiparameter_disease?

Bisaloo commented 6 months ago

I'm not a big fan of this since it would be an extra parameter (#106) with no actual function in the report. It would just be here to solve a technical gap but doesn't have any user-facing impact.

CarmenTamayo commented 6 months ago

So we could avoid adding an extra parameter if there was one called "disease_name" or something like that, which were used both to select the disease from the database and to discretise/plot the si?

Bisaloo commented 6 months ago

Okay, let's try this and see how it goes when updating the report.

joshwlambert commented 6 months ago

Would speeding up merging the upcoming plotting method for <epidist> into main be helpful here? It produces a very similar plot to the current {episoap} plot and can be modified as it returns a ggplot2 object to include the title and custom theme. It might also simplify writing the code as the plot method will work independent of whether the distribution is continuous or discretised (only difference is whether the plot is a smooth curve or bars).

CarmenTamayo commented 6 months ago

I would like to check it out to compare, this issue is probably resolved now that we've agreed to create epidists even for the case where the user doesn't extract the parameters from the epiparameter database, but still I'd like to see the plotting method and compare

CarmenTamayo commented 5 months ago

@joshwlambert could I ask what the status of this plotting method is?

Would speeding up merging the upcoming plotting method for <epidist> into main be helpful here? It produces a very similar plot to the current {episoap} plot and can be modified as it returns a ggplot2 object to include the title and custom theme. It might also simplify writing the code as the plot method will work independent of whether the distribution is continuous or discretised (only difference is whether the plot is a smooth curve or bars).

joshwlambert commented 5 months ago

It is still on a feature branch. I will work on this ASAP in order to merge into the main branch for testing. Will let you know once it is ready.

CarmenTamayo commented 5 months ago

thank you @joshwlambert