-
Notifications
You must be signed in to change notification settings - Fork 944
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
Allow optional ellipsoid definition to be passed to distance allowing for calculation types other than great circle #2476
base: master
Are you sure you want to change the base?
Conversation
Not sure if I need to request a review from yourselves. However noticed that there are several recent PRs that look like they're waiting on automated build results? From May onwards it seems. Maybe something amiss and blocking the CI chain? Thanks @JamesLMilner @twelch @stebogit @rowanwins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you're doing here @smallsaucepan with exposing a datum
option. I would like to see others feedback on this and taking geodesy as a dependency in turf
…d if they do perform an ellipsoidal distance calculation rather than the default great circle.
…al packages/turf/ conversion from mjs to rolled up js. "In strict mode code, functions can only be declared at top level or inside a block."
7b1be40
to
e6dfbd8
Compare
…meaning we can use import rather than require. Tightened up distance module function names and their optional parameters as well.
This PR should be good to go now. Uses geodesy 2.3.0. Bundle size increases in my build from 568083 to 584415 (about 3%). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @smallsaucepan for the thoughtful approach here and updates. I accept the use of the geodesy module to offer this functionality and potentially more in the future. I just have some questions/thoughts on cleaning up the interface/docs for using it. I appreciate your feedback.
@@ -14,6 +14,8 @@ import { | |||
Position, | |||
GeoJsonProperties, | |||
} from "geojson"; | |||
import type { Datum } from "geodesy"; | |||
import LatLonEllipsoidalVincenty from "geodesy/latlon-ellipsoidal-vincenty.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this module isn't exported out of the top-level of the geodesy library.
const datums = LatLonEllipsoidalVincenty.datums; | ||
export { datums }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this geodesy module defines only one datum, WGS84.
const datums = {
WGS84: { ellipsoid: ellipsoids.WGS84 },
};
https://github.com/chrisveness/geodesy/blob/master/latlon-ellipsoidal.js
So this would export just the one datum to users via @turf/helpers for use in the distance function correct? And to use it, the user would need to know to import this datum and pass it. Does this feel like the right interface for the users? Do they need this low level of access? Is the intention of exposing the datum objects to be able to offer more datums via https://github.com/chrisveness/geodesy/blob/master/latlon-ellipsoidal-datum.js?
What do you think about just accepting a 'wgs84'
string parameter in the distance function for the datum, which internally handles the geodesy
dependency. This could cause a breaking change in the future.
Alternatively, can you see how the documentation will be improved alongside this PR to tell users how to access/use a datum object? I can see the param doc at least points at the Helpers module.
* This uses the [Haversine formula](http://en.wikipedia.org/wiki/Haversine_formula) to account for global curvature. | ||
* Calculates the distance between two {@link Point|points}. | ||
* If a specific datum is passed, uses a geodesic ellipsoid calculation. | ||
* If no datum is passed, performs a great circle calculation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about keeping the ", using the Haversine formula". That seems to be a level of detail that people request having.
* Calculates the distance between two {@link Point|points} in degrees, radians, miles, or kilometers. | ||
* This uses the [Haversine formula](http://en.wikipedia.org/wiki/Haversine_formula) to account for global curvature. | ||
* Calculates the distance between two {@link Point|points}. | ||
* If a specific datum is passed, uses a geodesic ellipsoid calculation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are technically correct here in that if the user passes any valid datum object, that the code as it is now will run geodesicEllipsoidDistance
. But that doesn't seem clean/clear. Technically, I could possibly make up a datum and it will still run that one function right? But why? It seems like it would be better to accept the one expected ellipsoid datum, and throw an error otherwise. Or see my comment below about exposing just a string parameter to specify the datum.
Allow user to pass an optional ellipsoid definition to distance(), and if they do perform an ellipsoidal distance calculation rather than the default great circle. Partially resolves #1726 - length comes out as expected when users passes a WGS84 datum. This is likely the datum being used by the verification tool i.e. Qgis
Accuracy of nearestPointOnLine is a separate issue, largely unrelated to calculation method used by distance - see #1878 for why.
Can't use the latest version of the geodesy library due to ESM / CJS module loading issues :(
Please fill in this template.
npm test
at the sub modules where changes have occurred.npm run lint
to ensure code style at the turf module level.