covid-sim icon indicating copy to clipboard operation
covid-sim copied to clipboard

P.NC initialisation

Open galactic-src opened this issue 4 years ago • 5 comments

If you do not provide a density file, you hit an else block in SetupModel.cpp which starts by setting P.ncw = P.nch = (int)sqrt((double)P.NC); Unless I've missed something, at this point P.NC will be -1, as set unconditionally in ReadParams.

This code path is presumably unused, but it might be worth completely removing support for no density file. If keeping support for no-density-file simulation, perhaps it could use the cell width and the bounding box (generated from the population size) to calculate appropriate P.ncw and P.nch. In that case, it would be good to add a regression test which doesn't use a density file too.

galactic-src avatar May 24 '20 07:05 galactic-src

Do you mean the line in SetupModel.cpp?

zebmason avatar May 24 '20 07:05 zebmason

Yes, that's it.

galactic-src avatar May 24 '20 08:05 galactic-src

Fixed incorrect file name, thanks for highlighting.

galactic-src avatar May 24 '20 08:05 galactic-src

I've been putting together some minimal param files and hit this. (Most values mirror the examples, but I've decreased Infectious Period to avoid issues validating MAX_INFECTIOUS_STEPS and I reduced sampling time and population in the hope of getting something that runs quickly.)

While running, I see the following logged: Number of cells adjusted to be 0 (-2147483648^2). This number is the result of calling sqrt on -1 as this program demonstrates:

#include <math.h>
#include <stdio.h>
void main(int argc, char** argv) {
	printf("%d\n", (int)sqrt((double)-1));
}

Here are the param files I've been putting together in case you want to replicate:

pre-param file

[Include households]
0

[Include age]
0

[Kernel type]
2

[Kernel scale]
4000

[Infectious period]
5

param file

[Update timestep]
0.25

[Sampling timestep]
1

[Sampling time]
30

[Population size]
100

[Number of realisations]
5

[Initial number of infecteds]
3

[Reproduction number]
2.0

[Number of micro-cells per spatial cell width]
9

Generally I think it would be beneficial to support some smaller/simpler test cases anyway, and this is one rough edge in that story.

galactic-src avatar Jun 07 '20 14:06 galactic-src

Another observation I would like to raise here is that this particular computation should really be avoided in the first place (from #376).

The user should really only have to provide cell_grid_width and cell_grid_height, which specifies the dimensions of the simulation cell grid.

From that, the cells_count can be derived:

struct Param {
    // ... irrelevant fields omitted

    // they should be default-initialized to `0` to be some sentinel value that
    // does not make sense - you can't run the simulation with 0 grids?
    uint64_t cell_grid_width;
    uint64_t cell_grid_height;

    uint64_t cells_count() const {
    	assert (cell_grid_width > 0 && cell_grid_height > 0);
    	// should deal with uint64_t * uint64_t overflows
    	cell_grid_width * cell_grid_height
    }
}

In (int)sqrt((double)P.NC), the int cast truncates off the fractional parts (aka round down in this case).

jieyouxu avatar Jun 09 '20 10:06 jieyouxu