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

P.NC initialisation #337

Open
galactic-src opened this issue May 24, 2020 · 5 comments
Open

P.NC initialisation #337

galactic-src opened this issue May 24, 2020 · 5 comments

Comments

@galactic-src
Copy link
Contributor

galactic-src commented May 24, 2020

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.

@zebmason
Copy link
Contributor

Do you mean the line in SetupModel.cpp?

@galactic-src
Copy link
Contributor Author

Yes, that's it.

@galactic-src
Copy link
Contributor Author

Fixed incorrect file name, thanks for highlighting.

@galactic-src
Copy link
Contributor Author

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.

@jieyouxu
Copy link

jieyouxu commented Jun 9, 2020

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).

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

3 participants