spatialmodel / inmap

InMAP reduced-form air quality model for fine particulate matter (PM2.5)
GNU General Public License v3.0
61 stars 42 forks source link

allow population group specific mortality rates to used in the model #25

Closed dpaolella closed 7 years ago

ctessum commented 7 years ago

I fixed a couple of bugs. There's still one left which I think might be the same thing you fixed in a different place by just changing the test value.

dpaolella commented 7 years ago

Ok thanks, I will take a look!

ctessum commented 7 years ago

Also, if you run 'go generate' in the main directory it should regenerate the list of default output options.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.05%) to 81.163% when pulling b8ce872b037483f58b0b773fbcc06c5cbc6dd90b on mortData into 98209b235bf84bb6fbe564bb31b4458ef99efa5d on master.

dpaolella commented 7 years ago

Would you like to review before I merge? @ctessum

ctessum commented 7 years ago

Looks good. Go ahead.

On Mon, Jun 26, 2017, 3:49 PM David Paolella notifications@github.com wrote:

Would you like to review before I merge? @ctessum https://github.com/ctessum

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/spatialmodel/inmap/pull/25#issuecomment-311203480, or mute the thread https://github.com/notifications/unsubscribe-auth/AF6AAaM2bufukLmfoWCY809v-2UPXbMKks5sIDVcgaJpZM4N87FB .

dpaolella commented 7 years ago

I can't figure out what the error here is. It seems like there is something wrong with the file testSR.ncf. All tests pass when I run them locally. Do you have any idea what the problem might be?

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.05%) to 81.163% when pulling 5f392f1c922932975ebeca7ec61115fa5115a4fa on mortData into 98209b235bf84bb6fbe564bb31b4458ef99efa5d on master.

dpaolella commented 7 years ago

So...apparently changing the order of the mortality rates in the configExample.toml file resolved the issue, but that shouldn't make a difference because it's supposed to be read in as a map (and maps are unordered). It's confusing me that I don't get the error when I run tests locally and that I only get it for the SR test.

ctessum commented 7 years ago

It looks like the tests failed with version 1.7 of the Go compiler but not version 1.8. It's possible that the reason for this is that version 1.8 might iterate through map keys in a different order than version 1.7, and whether the tests pass is somehow dependent on that order. (The order of iterating through map keys is not guaranteed so we shouldn't depend on it.)

If this is the case, it can be fixed by forcing the indices to be in the same order every time, e.g., making sure that the mortality rate that comes first alphabetically gets index 0 and so on.

dpaolella commented 7 years ago

Oh! That's my bad. I totally knew that the iteration order for maps is not guaranteed. Thank you for the reminder.

dpaolella commented 7 years ago

I suspect there may be something else going on as well. The test was failing because the AllMort mortality rate was equal to 400 when it was supposed to be 800. All of the mortality rates in the mortality test shapefile are 800, so something else is causing the rates to be rewritten or modified. I'm still trying to figure out where/how this could be happening.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 81.191% when pulling 5ea9c912006783b9ad7a666177c4c95030dff5d7 on mortData into 98209b235bf84bb6fbe564bb31b4458ef99efa5d on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 81.191% when pulling f2b138a7420c26ad6ad55b1e93a4ed6dd0301a3a on mortData into 98209b235bf84bb6fbe564bb31b4458ef99efa5d on master.

dpaolella commented 7 years ago

I fixed the issue by making sure that the mortality rates are read in alphabetically like you suggested.