search

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 256 forks source link

Looks like the wrong scale in distance calculations (periodic) #447

Open zebmason opened 4 years ago

zebmason commented 4 years ago

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

Have if ((P.DoPeriodicBoundaries) && (P.indegrees.height ((double)abs(m % P.nch - l % P.nch)) > P.indegrees.height 0.5)) rather than if ((P.DoPeriodicBoundaries) && (P.incells.height ((double)abs(m % P.nch - l % P.nch)) > P.indegrees.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.

weshinsley commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

zebmason commented 4 years ago

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.