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

Allow optional ellipsoid definition to be passed to distance allowing for calculation types other than great circle #2476

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

smallsaucepan
Copy link
Member

@smallsaucepan smallsaucepan commented Aug 26, 2023

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.

  • [✓] Use a meaningful title for the pull request. Include the name of the package modified.
  • [✓] Have read How To Contribute.
  • [✓] Run npm test at the sub modules where changes have occurred.
  • [✓] Run npm run lint to ensure code style at the turf module level.

@smallsaucepan
Copy link
Member Author

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

Copy link
Collaborator

@twelch twelch left a 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

packages/turf-distance/index.ts Outdated Show resolved Hide resolved
…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."
…meaning we can use import rather than require. Tightened up distance module function names and their optional parameters as well.
@smallsaucepan smallsaucepan marked this pull request as ready for review September 14, 2023 03:45
@smallsaucepan
Copy link
Member Author

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%).

Copy link
Collaborator

@twelch twelch left a 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";
Copy link
Collaborator

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.

Comment on lines +126 to +127
const datums = LatLonEllipsoidalVincenty.datums;
export { datums };
Copy link
Collaborator

@twelch twelch Sep 18, 2023

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.
Copy link
Collaborator

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.
Copy link
Collaborator

@twelch twelch Sep 18, 2023

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.

@smallsaucepan smallsaucepan marked this pull request as draft September 23, 2023 00:32
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.

length and nearestPointOnLine - accuracy
2 participants