karimn / covid-19-transmission

0 stars 0 forks source link

Convolution process truncation for performance #5

Open karimn opened 4 years ago

karimn commented 4 years ago

@wwiecek I'm having doubts about how effective this would be. It would end putting zeros near the tail end of of the convolution discretized rev_gen_factor and rev_time_to_death, right? If you look at lines 227 and 235 in the main-karimn branch, you'll see that wouldn't really reduce computation by much. Maybe I'm missing something. What do you think?

wwiecek commented 4 years ago

There's 2 things:

I think the latter gives a more obvious speed-up (at the cost of discarding some information)

karimn commented 4 years ago

But that loop is needed to calculate the Rt, Rt_adj, new_cases, and mean_deaths. The number of iterations is based on the number of days we have data for.

wwiecek commented 4 years ago

But that loop is needed to calculate the Rt, Rt_adj, new_cases, and mean_deaths. The number of iterations is based on the number of days we have data for.

So it would calculate all of these quantities, but at a m-day interval (rather than 1-day).

Now that I write this, since the speed-up would be inversely proportional to m, maybe that is not so great. We maybe don't care if the model runs 8hrs instead of 16hrs?

karimn commented 4 years ago

Ok, I get it now!

From 16 to 8 would be a worthy improvement if possible.

wwiecek commented 4 years ago

Yes, but extra data formatting. We could have an option for the script which, if TRUE, groups days into consecutive 2-day pairs and 1) sums deaths, 2) averages mobility. Apart from changing N days in each subunit, we also need to pass a generating function which uses integral from -1 to 1 rather than -0.5 to 0.5. But other than that I think it's all the same?

wwiecek commented 4 years ago

PS: So this option could be good for troubleshooting. For new countries maybe running a more approximate model on full data in half/third/fifth of the time is beneficial, we can always run with full data later on.

karimn commented 4 years ago

Implementation changes to accommodate merging days:

karimn commented 4 years ago

This is now implemented but does not produce stable results. Converting this to a bug and putting it back on the todo list.