Remove implicit use of CRS in code base #1020
Conversation
Merge develop into branch
There was a problem hiding this comment.
Thank you for the lat lon check function! I think it is very valuable. I am not really sure in which other functions we could add it. What about:
latlon_to_geosph_vector(lat, lon, rad=False, basis=False)
lon_bounds(lon, buffer=0.0)
latlon_bounds(lat, lon, buffer=0.0)
dist_approx()
dist_to_coast_nasa()
These were all I could find in climada.util.coordinates that looked like they would require geographical coordinates. But maybe you thought about it and it does not make sense here?
I only have a few suggestions:
- describe the error message a bit better so the user has a better idea what could be wrong and what to do about it.
- change the range to -540 to 540 (apparently there is no convention in CLIMADA, and this seems soft enough as a error range)
- Could you restructure the PR description a bit? It was unclear what was done and what not. Also, the link to scrum number 129 is a link to issue 129 which I think is different? For the description, e.g.
Changes proposed in this PR:
- Add a cheap test for functions that must be in epsg:4326 but do take only value as input to catch when input coordinates are not geographic (e.g. in meters)
This PR treats scrum user story #129 Consistent use of CRS in CLIMADA
Further information:
...
Suggestions for future PRs
- Update functions that assume epsg:4326 without reason to be able to use other crs. (work in progress / requires more information)
climada/util/coordinates.py
Outdated
| Check if latitude and longitude arrays are likely in geographic coordinates, | ||
| testing if min/max values are within -90 to 90 for latitude and -180 to 540 | ||
| for longitude. | ||
| Returns True if lat/lon ranges are in the geographic coordinates range, otherwise False. |
There was a problem hiding this comment.
Can you use the numpy-style format for the docstring? See e.g. the next function latlon_to_geosph_vector
There was a problem hiding this comment.
Also, can you briefly explain in the docstring why the boundaries -180 (or -540, see comment below) to 540? I think it makes sense, but this choice should be motivated or at least explained for the user.
There was a problem hiding this comment.
Not sure the explanation for the 540<> makes sense, let me know if this is not the case.
climada/util/coordinates.py
Outdated
| @@ -1596,6 +1644,12 @@ def get_country_code(lat, lon, gridded=False): | |||
| return np.empty((0,), dtype=int) | |||
| LOGGER.info("Setting region_id %s points.", str(lat.size)) | |||
| if gridded: | |||
| # first check that input lat lon are geographic | |||
There was a problem hiding this comment.
why do we only check lat lon if gridded=True?
There was a problem hiding this comment.
Good question.. I think it should also test when gridded=False as it then calls get_country_geometries which requires WGS84. I changed it to test regardless of gridded.
Changes proposed in this PR:
This PR treats scrum user story #129 Consistent use of CRS in CLIMADA
Further information:
get_country_geometriesordist_to_coastthat are based on naturalearth data) at least test that input data seems geographic. Currently allowed values are -91 < lat < 90 and -181 < lon < 541 and maximum extension of 181 (latitudinal) and 361 (longitudinal) but it is not exactly clear to me where to do the cut-off. For instance, do we want to allow longitudinal extension greater than 360, or longitudinal values smaller than -180 (some tests related to that e.g.test_track_land_paramsare still failing due to longitudinal extension > 360)?trop_cyclonemodule but I was not exactly sure of what to do there. For instance, theget_close_centroidsdoes not check that input centroids and lat and lon from the TC track are geographic; I was not sure if this should be explicitly check or if it can be left as is.PR Author Checklist
develop)PR Reviewer Checklist