Repository navigation
Haversine formula fix - #2408
Conversation
| package geotrellis.util | ||
|
|
||
| object Haversine { | ||
| val EARTH_RADIUS = 6378137d // Use what gdal2tiles uses. |
There was a problem hiding this comment.
I'd suggest adding a doc comment declaring the units used.
| val EARTH_RADIUS = 6378137d // Use what gdal2tiles uses. | ||
|
|
||
| // (x: Double, y: Double) points | ||
| def apply(start: (Double, Double), end: (Double, Double), R: Double = EARTH_RADIUS): Double = { |
There was a problem hiding this comment.
For future GeoTrellisites, I'd suggest a doc comment indicating what the components of start and end are. e.g. (Latitude, Longitude), or (Longitude, Latitude) or (radius, angle), etc.
| implicit def jtsCoord2Point(coord: jts.Coordinate): Point = | ||
| Point(factory.createPoint(coord)) | ||
|
|
||
| implicit def pointToTuple2(point: Point): (Double, Double) = |
There was a problem hiding this comment.
While this is a nice convenience for people familiar with the particular code that translates between Point and Tuple2, I've been burned enough by unexpected or unintended conversions in the past to avoid this sort of thing personally, especially when the conversion is to a more general type. If you were converting between Point and something like Position(lat, lng) I'd be a bit less concerned because the translation semantics of x -> lat and y -> lng are clear, but _1 and _2 have no inherent semantics and therefore information is lost in the implicit conversion.
There was a problem hiding this comment.
Yes, i decided to remove this implicit at all as it is indeed ambiguous.
3ffde37 to
79dd8af
Compare
|
@metasim do you have any additional comments? |
79dd8af to
e28762f
Compare
3 participants
Fixes #2383