epiverse-trace / epiparameter

R package with library of epidemiological parameters for infectious diseases and functions and classes for working with parameters
https://epiverse-trace.github.io/epiparameter
Other
33 stars 11 forks source link

Possible name change for `vb_epidist` class #93

Closed joshwlambert closed 4 months ago

joshwlambert commented 1 year ago

The vb_epidist class currently has two slots called intrinsic_epidist and extrinsic_epidist. It has been pointed out that these might be unclear and perhaps human_epidist and vector_epidist would be clearer. I'm unsure of which is preferred. Please comment on this issue with any preference.

joshwlambert commented 9 months ago

Tagging with dev day for discussion on {epiparameter} development day.

joshwlambert commented 5 months ago

I'm questioning whether the {epiparameter} package should contain a <vb_epidist> class. I do not know of any current use cases where this class or it's functionality is used/required.

The <vb_epidist> plotting method was already removed in #315.

@Bisaloo and @chartgerink in terms of development I'm hoping to simplify and streamline {epiparameter} over the next few weeks. Do you agree with removing this?

@avallecam and @CarmenTamayo do you use <vb_epidist> in any scripts or training material?

Bisaloo commented 5 months ago

To clarify, epidemiological parameters for vector-borne diseases can still be included in epiparameter but as "simple" epidist objects. Is this correct?

So, what we would be losing here is the fact that the human_epidist and vector_epidist have been estimated together and it's probably unreliable to use human_epidist from one study with vector_epidist from another study. Am I getting this right as well?

joshwlambert commented 5 months ago

To clarify, epidemiological parameters for vector-borne diseases can still be included in epiparameter but as "simple" epidist objects. Is this correct?

Yes.

So, what we would be losing here is the fact that the human_epidist and vector_epidist have been estimated together and it's probably unreliable to use human_epidist from one study with vector_epidist from another study. Am I getting this right as well?

Yes, plus the S3 methods for the class which are apply the <epidist> methods to both the human and vector <epidist>s.

The reasoning behind adding the <vb_epidist> class was to allow intrinsic (human_epidist) and extrinsic (vector_epidist) to be jointly handled, with a planned addition for epidist_db() to return a <vb_epidist> if the two returned <epidist> objects are linked, but this never materialised.

The intrinsic and extrinsic parameters are linked by the extrinsic and transmission_mode in the <epidist> metadata.

For linking human and vector parameters from different studies, I think we can leave this to users of the package.

chartgerink commented 5 months ago

I'm all for streamlining in case keeps scope of the functionality the same. I did a bit of code-browsing and do not see any immediate issues.

As the maintainer @joshwlambert I trust your final judgment on the applicability :+1: Appreciate being pinged and looped in 😊

Bisaloo commented 5 months ago

I agree that using the epidist class for everything is reasonable then :+1:, with potentially a small note in a vignette highlighting the potential statistical pitfalls when not using the linked distribution.

avallecam commented 5 months ago

@avallecam and @CarmenTamayo do you use <vb_epidist> in any scripts or training material?

Not until now. We do not have any case study based on vector-borne diseases yet, as far as I'm aware. In order to provide informed feedback to this issue, we should plan to find a used case for this.

joshwlambert commented 4 months ago

Closing as this issue was addressed in #359.