mrc-ide / covid-sim

This is the COVID-19 CovidSim microsimulation model developed by the MRC Centre for Global Infectious Disease Analysis hosted at Imperial College, London.
GNU General Public License v3.0
1.23k stars 257 forks source link

Fix issue with initialisation of inv journey dists #411

Closed matt-gretton-dann closed 3 years ago

matt-gretton-dann commented 4 years ago

I noticed this whilst reviewing #400, and I'm not sure if it is a bug or not - @NeilFerguson I would appreciate your review.

I believe it doesn't affect any simulations published to date as it requires airports to be enabled - which they haven't been for CovidSim.

In the two loops starting at line 894 of src/CovidSim.cpp initialising InvJourneyDurationDistrib and InvLocalJourneyDurationDistrib, the variable j is initialised to 0 at the head of the loop and then used to find an appropriate index in the while loop, which is used to set the value of Inv...[i].

The issue is j is not reset each go round the loop. Therefore Inv...[i - 1] <= Inv...[i] (j is always increasing).

I am not sure whether this is a bug or not. I think it probably is and so have made the proposed change - but @NeilFerguson can you please confirm?

weshinsley commented 4 years ago

I suspect from lines 891 and 892, P.JourneyDurationDistrib[i] and P.LocalJourneyDurationDistrib[i] are cumulatively calculated, hence are monotonically increasing. (Or perhaps two entries might be the same, but never dropping).

Hence, I think you can resume the search with j where you last left it; it might/might not increase, since P.InvJourneyDurationDistrib will have 1024 elements, but JourneyDurationDistrib only 14.

But for PR #400, (if I'm right), that's a reason those two loops should not be fused together, or if they do, then they should have separate j1 and j2 keeping track of the progress through the cumulative distributions.

matt-gretton-dann commented 4 years ago

@weshinsley : Thanks for the review. So I'll "correct" the code by adding a comment that this is what is going on, and leave the code as originally written.

weshinsley commented 3 years ago

This was refactored at some point to make it more clear...