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

Looks like the wrong scale in distance calculations (periodic)

Open zebmason opened this issue 4 years ago • 4 comments

https://github.com/mrc-ide/covid-sim/blob/87127e715e20867071a9b41f8e68312edc58c4f1/src/Dist.cpp#L128

Have if ((P.DoPeriodicBoundaries) && (P.in_degrees_.height * ((double)abs(m % P.nch - l % P.nch)) > P.in_degrees_.height * 0.5)) rather than if ((P.DoPeriodicBoundaries) && (P.in_cells_.height * ((double)abs(m % P.nch - l % P.nch)) > P.in_degrees_.height * 0.5))

If it is a bug it dates back to prior to any of my refactorings and, I would hazard, not appear to affect any real world calculations.

zebmason avatar Aug 05 '20 16:08 zebmason

I think you're right, but will dig into this a bit - I believe this was a recent-ish fix put in place to handle that Russia and Alaska have cells with both positive and negative longitude.

weshinsley avatar Aug 06 '20 09:08 weshinsley

Sorry it's been so long - I am trying to figure all this out to review. Need to compare lines 114 and 128 to understand the top issue....

weshinsley avatar Nov 17 '20 10:11 weshinsley

P.ncw = I think number of cells (width), P.nch = number of cells (height ) - so possibly 114 should be talking about P.ncw too.

BUT - dist2_cc_min is only called in one place - kernel lookup tables - and as yet my param files are not causing it to be called at all...

weshinsley avatar Nov 17 '20 11:11 weshinsley

Lots of dead code knocking around - I couldn't understand the point of periodic boundaries. You have to track the conversions so that the expression has consistent units.

P.S. I belatedly realised that instances of Size should rather be DiagonalMatrix2 as they are really scalings for points relative to some bottom left corner.

zebmason avatar Nov 17 '20 18:11 zebmason