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.

Sign up for GitHub

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

Jump to bottom

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

Open
zebmason opened this issue Aug 5, 2020 · 4 comments · May be fixed by #444
Open

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

zebmason opened this issue Aug 5, 2020 · 4 comments · May be fixed by #444

Comments

@zebmason
Copy link
Contributor

zebmason commented Aug 5, 2020

covid-sim/src/Dist.cpp

Line 128 in 87127e7

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

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 zebmason linked a pull request Aug 5, 2020 that will close this issue
Open
@weshinsley
Copy link
Collaborator

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 weshinsley linked a pull request Aug 7, 2020 that will close this issue
Separate out the distance calculations #444
Open
@weshinsley
Copy link
Collaborator

weshinsley commented Nov 17, 2020 •
edited
Loading

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
Copy link
Collaborator

weshinsley commented Nov 17, 2020 •
edited
Loading

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Separate out the distance calculations zebmason/covid-sim
2 participants
@weshinsley @zebmason