-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Truncation adjustment does not work as expected if truncation vector is too long #735
Comments
Thanks for the detailed report @kgostic - this sounds like a frustrating bug. I think this issue is related to the truncation functionality itself (based on this check
and then this indexing
This is in effect slicing off the back of the truncation vector and not the front due to it being reversed. I think this https://github.com/epiforecasts/EpiNow2/pull/736/files fixes the problem but will investigate in more detail + add tests etc shortly. I think we should also open another issue to do a wider dimension check to make sure this issue isn't elsewhere in the code base (i.e here
ylin I think this doesn't make sense (though in the way it is used for our convolutions this can't currently be the case as xlen = len ).
As a more general comment I think I might set the threshold of the delay PMF to be lower as having so many entries with such low weight (i.e <1e-6) is going to lead to a lot of computation that doesn't add a great deal to the results.
What do we think about still adding a warning message here? I'm not sure its a terribly good idea for expected practice to be to enter delays longer than the data but on the other hand in say a outbreak setting this might be hard to avoid. |
Ah yes that makes sense, and I agree with the fix.
I agree, this makes sense
Note you can use the
A warning seems appropriate to me. |
So that does look like it was the problem. Urgh! Thanks again for the clear report. This issue should closed by the following actions:
I am not sure what the current release time line is but I think we are close to another small patch release and so this should be on CRAN by September I would guess (CRAN is currently on Holiday). |
Makes sense. Thanks again for the report. |
Summary:
I am passing a truncation adjustment using a vector,
truncation_pmf
withtrunc_opts(dist = NonParametric(truncation_pmf))
. If thetruncation_pmf
vector is longer than the data timeseries, the truncation adjustment does not work as expected.Description:
I am working with a dataset in which it takes several months for reporting to be complete. However, in order to keep our runtimes reasonable, we don't usually input more than 8 weeks of data at a time into EpiNow2. This leaves us in a situation where the maximum reporting delay (70d, chosen to capture 99% of the delay probability mass) is greater than the length of our data timeseries (56d).
Without thinking hard enough about potential dimension mismatch issues, we had been trying to pass in the full delay pmf (which has length 71) into EpiNow2 using
trunc_opts(dist = NonParametric(truncation_pmf))
. The delay pmf was longer than the data timeseries, but EpiNow2 ran without any errors and or convergence issues.However, we later realized that the right truncation adjustment wasn't working as expected. There was little if any meaningful difference between the corrected
imputed_obs
and uncorrectedobs_reports
estimates, even though we were expecting a correction of about 50% on the most recent date. At first we had no idea why this was. We assumed we had passed in the wrong truncation pmf at first, but saw something that mostly resembled our inputs in thefit$estimates$args
metadata, and became even more confused.We ultimately realized there might be a dimension mismatch issue, and were able to fix the problem by shortening the delay pmf.
This was a silent error that took us a few days to figure out. It would be really great if it's possible to add some kind of validator or error message to check the truncation pmf input if there are dimension limits. Even better would be to ensure that if too long of a pmf is passed in, the right hand side is trimmed but the left hand side (where most of the delay mass resides) is left intact.
Reproducible Steps:
EpiNow2 Version:
1.5.2
This screenshot shows that when we pass a shorter truncation vector, the correction at the end of the time series occurs, but it is missing when we pass a vector that is too long:
The text was updated successfully, but these errors were encountered: