added get_bounds functions for EPSG 4326 #980
Conversation
spjuhel
left a comment
There was a problem hiding this comment.
This looks good overall,
I made some minor suggestions, you may choose to ignore (except maybe the additional tests).
| ), | ||
| (175, -20, 530, 90), | ||
| ) | ||
|
|
There was a problem hiding this comment.
You should add tests for invalid bounding box.
Co-authored-by: Samuel Juhel <10011382+spjuhel@users.noreply.github.com>
climada/util/coordinates.py
Outdated
| # print warning if ISO code not recognized | ||
| for country_name in country_names: | ||
| if not country_name in nat_earth[["ISO_A3", "WB_A3", "ADM0_A3"]].values: | ||
| LOGGER.warning(f"ISO code {country_name} not recognized.") |
There was a problem hiding this comment.
wouldn't it be better to simply throw an exception in case the country is not recognized?
I can't see the point of filtering out the unrecognizable countries in here. If a user runs this with a typo in one of the country names would they be upset if it fails?
There was a problem hiding this comment.
Yes I agree, this would be better. I changed it to raise a ValueError now.
My only doubt is that the function util.coordinates.get_country_geometries() existed before and if you input a list of ISO codes with one invalid code, it did NOT raise an error before (this is why I "only" added a warning). Wouldn't this be a potential problem for existing code to break down due to the error, or is it better to make aware that the input was not correct?
There was a problem hiding this comment.
Good question. Imo, it is better to make them finally aware that their input has been wrong all the time.
We just need to add a note in the change log.
There was a problem hiding this comment.
Perfect, agreed. I'm adapting the change log accordingly.
Changes proposed in this PR:
bounding_box_global,bounding_box_from_countries, andbounding_box_from_cardinal_boundsin theclimada.util.coordinatesmodule.Notes
PR Author Checklist
develop)PR Reviewer Checklist