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.

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

added get_bounds functions for EPSG 4326 #980

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

ValentinGebhart
Copy link
Collaborator

@ValentinGebhart ValentinGebhart commented Nov 27, 2024

Changes proposed in this PR:

  • Introduced three new utility functions global_bounding_box , get_country_bounding_box and bounds_from_cardinal_bounds in the climada.util.coordinates module.
  • Added unit tests.

Notes

  • Methods only treat EPSG 4326 coordinate system. For more flexible methods, a bounds dataclass or class could be implemented in a future PR
  • These methods were implemented for the Seasonal Forecast module but moved here for general uses

PR Author Checklist

PR Reviewer Checklist

@ValentinGebhart ValentinGebhart marked this pull request as draft November 27, 2024 17:07
@ValentinGebhart ValentinGebhart marked this pull request as ready for review November 29, 2024 16:37
Copy link
Collaborator

@spjuhel spjuhel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall,
I made some minor suggestions, you may choose to ignore (except maybe the additional tests).

climada/util/coordinates.py Outdated Show resolved Hide resolved
climada/util/coordinates.py Outdated Show resolved Hide resolved
),
(175, -20, 530, 90),
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add tests for invalid bounding box.

Co-authored-by: Samuel Juhel <10011382+spjuhel@users.noreply.github.com>
# 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.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants