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

Haversine distance should use spherical Earth radius #26

Open
1ec5 opened this issue Dec 15, 2017 · 5 comments
Open

Haversine distance should use spherical Earth radius #26

1ec5 opened this issue Dec 15, 2017 · 5 comments

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 15, 2017

This library currently defines the radius of the Earth as 6 373 000 m for the purpose of converting between meters and radians in the Haversine formula:

let metersPerRadian = 6_373_000.0

Apparently this value came from an old version of turf-distance that I originally ported to an internal Swift application, before it got copied into another internal Swift application, then copied into the navigation SDK, then moved here. It’s close to the value of 6 372 797.560 856 that Osmium describes as “Earth’s quadratic mean radius for WGS84”.

These days, Turf.js uses a spherical approximation of 6 671 008.8 m for its radian-to-meter conversion and Haversine formula. The Haversine formula was always meant to be used with a spherical meters-per-radian value. We should probably change this value to match Turf.js. 🙀

/ref Turfjs/turf#978 Turfjs/turf#1012 Turfjs/turf#1176 #21 (comment)
/cc @frederoni @bsudekum @captainbarbosa

@bsudekum
Copy link

Do you think this could have any impact on mapbox/mapbox-navigation-ios#901?

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 15, 2017

Do you think this could have any impact on mapbox/mapbox-navigation-ios#901?

Yes, that issue could well be caused by a discrepancy between OSRM and Turf that builds up as per-segment and per-step distances are summed together. Differences in coordinate precisions could also play a role.

OSRM uses an Earth radius of 6 372 797.560 856 when calculating distances:

https://github.com/Project-OSRM/osrm-backend/blob/a53794f8645df783129fd48bda7a9c1e1df072fd/include/engine/routing_algorithms/routing_base.hpp#L322
https://github.com/Project-OSRM/osrm-backend/blob/a53794f8645df783129fd48bda7a9c1e1df072fd/include/engine/routing_algorithms/routing_base.hpp#L345
https://github.com/Project-OSRM/osrm-backend/blob/088d4edc6b761d2642a9b1ab7d85b701d3165fb2/include/util/coordinate_calculation.hpp#L26-L28

This is the “groundtruth” value, but @mourner makes the point in Turfjs/turf#635 (comment) that Haversine distance calculations (as opposed to more rigorous distance calculations) rely on a spherical approximation of 6 671 008.8 m. Some more context on OSRM’s choices in Project-OSRM/osrm-backend#3567.

We should change this library’s Earth radius value either way, but I don’t know which is better. Turfjs/turf#1176 proposes allowing clients to supply their own Earth radius, which could be a reasonable approach.

/cc @daniel-j-h @TheMarex

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 20, 2018

For comparison, CLLocation.distance(from:) uses an Earth radius of 6 378 137 m:

let nullIsland = CLLocation(latitude: 0, longitude: 0)
let antipode = CLLocation(latitude: 0, longitude: 180)
let radius = nullIsland.distance(from: antipode) / Double.pi

We could also replace the haversine formula with the Vincenty formula, which is super precise over long distances. However, I think aligning the Earth radius value to either Turf.js or OSRM (WGS84) would be more desirable for Turf.swift, considering that clients such as the navigation SDK need to interoperate with those libraries.

@1ec5 1ec5 added the JS parity label Jan 24, 2018
@1ec5
Copy link
Contributor Author

1ec5 commented Apr 23, 2018

OSRM uses an Earth radius of 6 372 797.560 856 when calculating distances

There’s some discussion about whether OSRM’s value is correct: Project-OSRM/osrm-backend#5051.

@danpat
Copy link

danpat commented Apr 23, 2018

Also note that OSRM is testing out cheap-ruler as a way to get more accuracy for fewer CPU cycles, so its use of Haversine might go away completely.

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

No branches or pull requests

3 participants