trop_cyclone: fix for tracks crossing antimeridian #839
Conversation
tovogt
left a comment
There was a problem hiding this comment.
See below for comments that explain some of the changes directly in the code.
|
GitHub tests pass, but Jenkins tests fail with |
Thanks for checking! Here is the reason, from the Jenkins log output:
Hence, the IBTrACS file in Jenkins is really old (2020!), and needs an update. |
|
The file is updated, and hence the test climada_branches passes, but the petals_compatibility test still fails with |
Good point, that means that the PR CLIMADA-project/climada_petals#105 is actually a compatibility PR and needs to be merged at the same time with this one. |
peanutfun
left a comment
There was a problem hiding this comment.
Great to see that this now works, thanks for adding the necessary unit tests! 🙌
I am a bit confused about functions returning both centroids and masks for centroids. I think they should return only one.
I agree with most of the naming changes, and will simply resolve the respective conversations 😇
|
Thanks for the review! Yes, understanding masks can be challenging in this module. For performance reasons, we have a mask for each track position, but at some point we also apply a single mask to the centroids to filter out centroids that are not affected by any of the track positions. But we need the masks to stick around in order to inflate the result (that is only defined on the masked centroids) to the original set of centroids in the end. |
|
@tovogt The double-backticks are very annoying and easy to miss 😅 From my point of view, this is ready! Can I merge? |
|
Wait a second, CHANGELOG... And let's also merge CLIMADA-project/climada_petals#105 soonish due to compatibility. |
|
Okay, ready to be merged! |
|
@emanuel-schmid Thanks for fixing the CHANGELOG! I should have merged develop before adding the new points... |
|
@tovogt @emanuel-schmid Nice work, thank you! |
Changes proposed in this PR:
TropCycloneto cover the case where a TC track crosses the antimeridian. Note that this test fails in the current develop branch!TropCycloneto make the new test pass. This mostly affects the helper functionget_close_centroidsand surrounding code. Note that this function is also used in petals, and there is a compatibility PR here: tc_rainfield: tests for tracks crossing the antimeridian climada_petals#105trop_cyclone.pyto make binary numpy arrays start withmask_and arrays of indices withidx_. I found the previous nomenclature confusing while working on this fix.util.coordinates.latlon_boundsto cover the case where the specified buffer is very large so that the bounds cover more than the full longitudinal range [-180, 180].This PR does NOT fix #831!
PR Author Checklist
develop)PR Reviewer Checklist