Skip to content
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

incDeath_X output contains cumDeath_X values? #381

Open
bwynneHEP opened this issue Jun 11, 2020 · 3 comments
Open

incDeath_X output contains cumDeath_X values? #381

bwynneHEP opened this issue Jun 11, 2020 · 3 comments

Comments

@bwynneHEP
Copy link
Collaborator

I've been looking at some of the severity data broken-down into the different age groups. I noticed a minor (but annoying) bug in one of the main outputs I plot: the incDeath_X output, where X takes values from 0 to 16 corresponding to the different age groups.

It seems that these incidence values output the exact duplicate of the cumulative values, and so they rise monotonically throughout a simulation. This does not appear to be true for incDeath_ILI_X, incDeath_SARI_X, or incDeath__Critical_X, and so one can presumably reconstruct the correct value by summing over these three outputs instead. Nonetheless, it would be helpful for the combined value output to be corrected.

I note that the outputs are created here:
https://github.com/mrc-ide/covid-sim/blob/master/src/CovidSim.cpp#L4083
using the TSMean[i].incDa[j] value.

Looking at where this value is set:
https://github.com/mrc-ide/covid-sim/blob/master/src/CovidSim.cpp#L5033
TimeSeries[n].incDa[i] += (double)StateT[j].cumDa[i]; would appear to be the source of the mistake. However, I note many other values set in a similar way, so perhaps I have misunderstood the calculation?

This seems to only be an issue affecting the formatting of simulation output, and I doubt it would have affected anyone's results without them noticing.

@dlaydon
Copy link
Collaborator

dlaydon commented Jun 15, 2020

Hi @bwynneHEP and @weshinsley, yes you're right this does actually output the cumulative incidence. It was a quick hack that we post process in R. However you're right we should change it. Before I do though I need to check that it doesn't ruin anything upstream. Will get to it soon.

@bwynneHEP
Copy link
Collaborator Author

Not an urgent problem - as you say, once I knew about it, fixing as a post-process step is easy.

Probably worth a careful look though, as there seemed to be a few incSomething values that were set a similar way (i.e. without the subtraction of the previous step value before the addition of the new one).

I also noticed that the corresponding cumDeath_X output is made by summing over the ILI, SARI and Critical categories at the moment the output is made inside CovidSim.cpp. This might mean that some other ad-hoc fixes have already been applied.

@dlaydon
Copy link
Collaborator

dlaydon commented Jun 15, 2020

Yes when we do it properly, we add to the cumulative incidence at every timepoint in the State/StateT/POPVAR structures, then record the incidence in TimeSeries/RESULTS structure in RecordSample / CovidSim.cpp by subtracting the previous timepoint's running total from the current running total. All a little cumbersome but as it works who really cares?! Would be interested to see a cleaner version that preserves the threading if you're interested (no worries if not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants