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

util.coordinates: remove broken dist_to_coast function #840

Merged
merged 7 commits into from
Apr 15, 2024

Conversation

tovogt
Copy link
Collaborator

@tovogt tovogt commented Jan 17, 2024

Changes proposed in this PR:

  • Always use precomputed distances to coast in Centroids.set_dist_coast to avoid Persistent Wind Intensity Patterns in Historical Typhoon Tracks Beyond 180 Degrees Longitude #831 and similar issues with the implementation of util.coordinates.dist_to_coast (it is also quite slow, actually).
  • Remove the util.coordinates.dist_to_coast function because it is now used nowhere.
  • It also removes some helper functions that were only used in dist_to_coast, further reducing the amount of code in util.coordinates.

This PR fixes #831, see CLIMADA-project/climada_petals#106 for a follow-up PR in petals.

PR Author Checklist

PR Reviewer Checklist

@tovogt tovogt mentioned this pull request Jan 25, 2024
13 tasks
Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

Nice. While removing code is great, I would like to keep the public functions and add deprecation warnings. This way, we do not habe a major change following semantic versioning. I am fine with removing utm_zones though, seems like a really obsolete function to me.

tovogt and others added 2 commits February 21, 2024 11:37
Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
Copy link
Collaborator Author

@tovogt tovogt left a comment

Choose a reason for hiding this comment

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

I added dist_to_coast, but would still opt to remove get_coastlines since it's clearly a helper function for the distance computations.

@emanuel-schmid
Copy link
Collaborator

Many thanks! I'd suggest to wait with merging though until 5.0 🤔

@tovogt
Copy link
Collaborator Author

tovogt commented Apr 15, 2024

Okay, this is ready to be merged.

@emanuel-schmid If we merge this before CLIMADA-project/climada_petals#122 is merged, we can also ditch CLIMADA-project/climada_petals#106 and instead include the necessary changes directly in CLIMADA-project/climada_petals#122. That's a bit easier, since CLIMADA-project/climada_petals#106 is only about a few lines anyway, and all of the affected lines will also be affected by CLIMADA-project/climada_petals#122

@emanuel-schmid emanuel-schmid merged commit 37b1e5e into develop Apr 15, 2024
11 of 12 checks passed
@emanuel-schmid emanuel-schmid deleted the feature/remove_dist_to_coast branch April 15, 2024 09:31
gdeskos pushed a commit to gdeskos/climada_python that referenced this pull request Sep 16, 2024
…ct#840)

* util.coordinates: remove dist_to_coast function

* Update climada/hazard/centroids/centr.py

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>

* u_coord: dist_to_coast as wrapper for dist_to_coast_nasa

* Update climada/hazard/centroids/centr.py

Fix trailing whitespace

* Centroids.get_dist_coast: fix tests

---------

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>
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