idem-lab / epiwave.params

0 stars 0 forks source link

create_epiwave_massfun not working #3

Closed SeniorKate closed 2 weeks ago

SeniorKate commented 3 weeks ago

Create_epiwave_massfun seems to be shifting everything back by one day which causes problems (for example it gave my gamma distribution a value for day 0, which is not supposed to be allowed).

We thinking it is something to do with how mass is distributed across the sequence of values between min and max delays

possibly relatedly, the convolution matrix on generation interval is broken (infectiousness is zero) and giving reff estimates in the trillions.

SeniorKate commented 3 weeks ago

ecdf that went into the function vs pmf that came out (note the top starts on day 1 and the bottom on day 0)

ecdf pmf

AugustHao commented 2 weeks ago

side note: compare distributional::cdf versus distributional::density, the cdf approach (aka ours) is a better discretisation of continuous distributions, so we should stick to the existing method in https://github.com/idem-lab/epiwave.params/blob/6faa4b3d9eef077a51f5652f1c7ec115be407bcb/R/create_epiwave_massfun.R#L15C1-L22C6

AugustHao commented 2 weeks ago

to fix this issue, we simply need to change these two lines: https://github.com/idem-lab/epiwave.params/blob/6faa4b3d9eef077a51f5652f1c7ec115be407bcb/R/create_epiwave_massfun.R#L19-L20

from

upper = cdf_fun(delays + 1), lower = cdf_fun(delays),

to

upper = cdf_fun(delays), lower = cdf_fun(delays - 1)

however, we should be clear in documentation what this means: now the function outputs delay probability mass from one discrete time step before delays up to the time step of delays. E.g., if delays = 3 days, then the output mass corresponds to the probability of a delay between 2 and 3 days.

Previously, the function outputted delay probability mass from delays up to one discrete time step after delays. E.g., if delays = 3 days, then the output mass corresponds to the probability of a delay between 3 and 4 days.

AugustHao commented 2 weeks ago

in other words, the current function isn't broken, but the indexing of delays time is inconsistent with how the convolution matrix is time-indexed in discrete time.

AugustHao commented 2 weeks ago

fixed in this change in 4f740c8