mapbox / turf-swift

A Swift language port of Turf.js.
https://mapbox.github.io/turf-swift/
ISC License
237 stars 55 forks source link

Haversine distance should use spherical Earth radius #26

Open 1ec5 opened 6 years ago

1ec5 commented 6 years ago

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:

https://github.com/mapbox/turf-swift/blob/40c7d772c46837aa9e5ceb60d22d8ad97a4be708/Turf/Turf.swift#L7

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 https://github.com/mapbox/turf-swift/pull/21#discussion_r157246151 /cc @frederoni @bsudekum @captainbarbosa

bsudekum commented 6 years ago

Do you think this could have any impact on https://github.com/mapbox/mapbox-navigation-ios/issues/901?

1ec5 commented 6 years ago

Do you think this could have any impact on https://github.com/mapbox/mapbox-navigation-ios/issues/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 https://github.com/Turfjs/turf/issues/635#issuecomment-292011500 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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.