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

Clarify that current cross-track distance uses spherical (Haversine) earth model, add Ellipsoidal (Vincenty) version #1140

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

nrhill1
Copy link

@nrhill1 nrhill1 commented Jan 16, 2024

Issue #1128

In response to Issue #1128 , I moved the old cross_track_distance.rs into a new cross_track_distance_haversine.rs file and added an ellipsoid cross track distance function in cross_track_distance_geodesic.rs that uses the Karney 2023 improvements to the BML algorithm on geodesic intersections.

Copy link

@busstoptaktik busstoptaktik left a comment

Choose a reason for hiding this comment

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

Conceptually, I think this material would work better as a trait implemented for the Geodesic struct. As an example, I have a draft of a very different (and most likely much worse) cross-track-distance approach over at Geodesy, where it is implemented for the Ellipsoid struct (and Geodesic is really just a glorified Ellipsoid).

It would allow the functionality to work for any ellipsoid, not just the WGS84, which you constrain it to here, and it would eliminate the (rather heavy) recomputation of all the numerical factors for Geodesic at every new call.

That said, I'm just a humble hangaround, not a member of the Georust gang, so the real georusters may have a different view of this

@nrhill1
Copy link
Author

nrhill1 commented Feb 28, 2024

@busstoptaktik -- Interesting take, Thomas. I am, however, struggling to find the Geodesic struct in this crate so that I can understand your point a little better.

@busstoptaktik
Copy link

struggling to find the Geodesic struct in this crate so

I'm talking about the Geodesic struct from geographiclib_rs, which you use in your code

@nrhill1
Copy link
Author

nrhill1 commented Mar 1, 2024

So @busstoptaktik; are you suggesting we put these as a trait implementations in geographiclib_rs?

@nrhill1 nrhill1 changed the title Nrhill1 issue1128 patch1 Clarify that current cross-track distance uses spherical (Haversine) earth model, add Ellipsoidal (Vincenty) version Mar 1, 2024
@busstoptaktik
Copy link

are you suggesting we put these as a trait implementations in geographiclib_rs?

Since geographiclib-rs is a hard dependency anyway, and since it extends it with material based on additional work by the original author of GeographicLib, I think it would be a reasonable place to put it.

Also note that it would be meaningful (e.g. for roundtripping), and probably almost free, to return a tuple consisting of the across track distance in combination with the along track distance for the intersection.

@michaelkirk
Copy link
Member

Thank you for the code and sorry that this sat here for so long!

ses the Karney 2023 improvements to the BML algorithm on geodesic intersections.

Is this based on reading a paper or on existing code somewhere? Can you provide a link to either or both?

it would eliminate the (rather heavy) recomputation of all the numerical factors for Geodesic at every new call.

Note that Geodesic::wg84() is memoized, so it only incurs that setup cost one time.

@busstoptaktik's larger point about being able to use the method on other geodesics is reasonable, but in any case, I think we'll still want this existing API as well, because it mirrors our Haversine implementation, so I think it makes sense to merge this.

(I still do want to update the docs and compare to any reference implementation you can link before merging)

@nrhill1
Copy link
Author

nrhill1 commented Mar 9, 2024

Hi @michaelkirk, thanks for joining us!

I've implemented the ellipsoidal formula based on the following Python code
which is related to the changes in this Karney 2023 paper, as well as an existing Rust implementation from Catenary.

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