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

Add rhumb-line operations #1090

Merged
merged 3 commits into from
Oct 17, 2023
Merged

Add rhumb-line operations #1090

merged 3 commits into from
Oct 17, 2023

Conversation

apendleton
Copy link
Contributor

@apendleton apendleton commented Oct 16, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This PR adds a bunch of operations that are roughly equivalent to most of the current Haversine operations in geo, but using rhumb line (a.k.a. "loxodrome") geometry instead of great-circle geometry. In particular, this PR adds RhumbBearing, RhumbDestination, RhumbDistance, RhumbIntermediate, and RhumbLength traits.

The code layout is a little different from the haversine ones (everything is in a new rhumb module), mainly because I wanted to share code between several of these since there's a lot of overlap. In particular, all of the operations that work on two points (so, bearing, distance, and intermediate) use a common calculations struct that does a bunch of the up-front shared setup. Obviously this is all rearrange-able though if there's a different preferred layout.

The RhumbBearing, RhumbDestination, and RhumbDistance implementations are heavily inspired by their Turf.js equivalents (bearing, destination, and distance, respectively), which seem in turn to largely be inspired by the Movable Type rhumb line formula descriptions.

Turf doesn't have an intermediate operation for rhumb lines, so what's I've done here is "find the rhumb distance and rhumb bearing, calculate a fraction along that distance, and then calculate a rhumb destination," with (hopefully) most of the redundant trig factored out, as there's a fair bit of overlap between those operations. There might well be a more-direct trigonometric solution to this problem, though? Movable Type has a "rhumb midpoint" formula, but it doesn't seem to generalize to fractions other than 0.5; pygeodesy does seem to implement this concept, and special-cases 0.5 with the formula from Movable Type but also has a more general one, but honestly I found the code to be very difficult to understand. Hopefully what I've got is good enough for a first pass.

Tests are mostly appropriated from the equivalent Haversine traits and then adjusted so the values look right. I also separately confirmed that at least for the functions where Turf has equivalents, these implementations produce (approximately, allowing for float differences, etc.) the same values.

A couple of other odds and ends:

  • Naming and methodology: all of the methods here use spherical geometry. Iterative methods for calculating these values on an oblate spheroid also exist (see, e.g., the ones in GeographicLib), so there may be some future in which it might make sense to include those as well, as with HaversineX vs GeodesicX; if so, it may make sense to do something more qualified than RhumbX to make it clear what kind of rhumb/loxodrome operations these are? I'm not sure what that would be though.
  • Attribution: as these are ports and refactors of existing code, I'm not sure if I should put references in comments in these files to where they came from and what their licenses originally were. It doesn't look like existing code that's of similar provenance does so now for the most part, but if that would be a good thing to do, let me know where it's preferred that it go, and I can add it.

@urschrei
Copy link
Member

Attribution: as these are ports and refactors of existing code, I'm not sure if I should put references in comments in these files to where they came from and what their licenses originally were. It doesn't look like existing code that's of similar provenance does so now for the most part, but if that would be a good thing to do, let me know where it's preferred that it go, and I can add it.

Any time this question arises, I think about https://macwright.com/2013/02/18/literate-jenks (Even though nobody should be using Jenks in this day and age, but I digress). So in answer to your question: you never know who's going to be spelunking in this code in 30 years, so any breadcrumbs you can leave will be appreciated – if you have time, please add them.

Otherwise, this seems like a clean well-organised module to me, and a great addition to the crate. Thank you!

let delta_phi = delta * theta.cos();
let mut phi2 = phi1 + delta_phi;

// check for some daft bugger going past the pole, normalise latitude if so
Copy link
Member

Choose a reason for hiding this comment

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

I assume this comment is copied verbatim from the movable-type implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it was in the movable-type one and Turf kept it, so I did too. Happy to clean it up though if you prefer something more... professional 😅.

Copy link
Member

Choose a reason for hiding this comment

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

Oh no, was just checking!

@apendleton
Copy link
Contributor Author

@urschrei added an attribution block to rhumb/mod.rs .

Co-authored-by: Corey Farwell <coreyf@rwell.org>
@urschrei urschrei added this pull request to the merge queue Oct 17, 2023
Merged via the queue into georust:main with commit 173085f Oct 17, 2023
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