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

Update distance measurement factors #635

Closed
dhivehi opened this issue Mar 28, 2017 · 21 comments
Closed

Update distance measurement factors #635

dhivehi opened this issue Mar 28, 2017 · 21 comments
Assignees
Labels

Comments

@dhivehi
Copy link
Collaborator

dhivehi commented Mar 28, 2017

Turf line distance calculation is not accurate according to google earth. Is there any way to use custom radius in calculation?

@DenisCarriere
Copy link
Member

Please provide more context for you issue.

@turf/distance is using the Haversine formula which takes into consideration the curvature of the earth.

Closing this issue.

@dhivehi
Copy link
Collaborator Author

dhivehi commented Mar 28, 2017

its not the formula issue, but the problem is the earth radius being used is not the radius used in google earth.

@dhivehi
Copy link
Collaborator Author

dhivehi commented Mar 28, 2017

Official google earth's radius is 6378137meters.
I'm guessing this code is the problem.

var r={miles:3960,nauticalmiles:3441.145,degrees:57.2957795,radians:1,inches:250905600,yards:6969600,meters:6373e3,metres:6373e3,kilometers:6373,kilometres:6373,feet:20908792.65}

@DenisCarriere
Copy link
Member

@DenisCarriere DenisCarriere changed the title Distance not accurate Update distance radius according to Google's official earth radius Mar 28, 2017
@DenisCarriere DenisCarriere reopened this Mar 28, 2017
@dhivehi
Copy link
Collaborator Author

dhivehi commented Mar 29, 2017

I have send a PR, but it failed for some reason. I donno why. I'm new to github PR. This was my 1st PR in github.

@dpmcmlxxvi
Copy link
Collaborator

My guess would be that, since the helper functions are used everywhere, changing their numerical behavior has affected unit tests throughout the entire library. One of the errors on travis-ci is an expected output not matching a fixture. Changing these constants will probably require going through and addressing all downstream unit tests that depend on the helper functions that rely on these constants.

@dhivehi
Copy link
Collaborator Author

dhivehi commented Mar 29, 2017

So could you suggest a fix or can u fix it?

@dpmcmlxxvi
Copy link
Collaborator

dpmcmlxxvi commented Mar 29, 2017

I suggest you look at the travis-ci report and inspect at each unit test error returned in your PR. The error tells you what was the expected value and what was the actual value. Track down the cause of the difference and you'll likely see that some distance calculation is the root cause. Then update those tests.

@DenisCarriere
Copy link
Member

@dpmcmlxxvi Exactly.

@dhivehi I pushed a commit to your PR b7e826a which should of updated the the libraries that depend on @turf/helpers & @turf/distance.

The only things that really changed was the factors from your commits a65c237.

var factors = {
    miles: 3963.190592,
    nauticalmiles: 3443.918467,
    degrees: 57.2957795,
    radians: 1,
    inches: 251107755.9055118,
    yards: 6975215.442,
    meters: 6378137,
    metres: 6378137,
    kilometers: 6378.137,
    kilometres: 6378.137,
    feet: 20925646.325459316
};

CC: @tmcw @morganherlocker

@dhivehi
Copy link
Collaborator Author

dhivehi commented Mar 30, 2017

So does this means now the issue is fixed?

@DenisCarriere
Copy link
Member

It will be fixed in the next minor/patch release, just waiting for a response from @tmcw or @morganherlocker (if it's OK with them I can submit the release).

The @turf/distance & @turf/helpers affect many modules outside of Turf, I don't want to release this yet until I've got the 👍 from someone else.

@dhivehi
Copy link
Collaborator Author

dhivehi commented Mar 30, 2017

Ok. Thanks

@morganherlocker
Copy link
Member

I am 👍 on making turf-distance more accurate, but it will require a major @turf/turf version, because it will be a breaking change at the API level. Also please note, that this change does not mean we will officially support "Google" distance in any way. We will not make any change to match a particular 3rd party software's behavior and would not classify a future drift in behavior as a bug. In fact, I would prefer a different source for this measurement that is independent of any vendor to avoid any misunderstandings around expectations.

@DenisCarriere
Copy link
Member

DenisCarriere commented Mar 30, 2017

@dhivehi Can you provide some other sources that confirm these distance factors.

@morganherlocker Should we revert the PR & make a Turf v4.0 branch with the changes until we are ready to publish another major release.

OR

Keep it merged and not publish a release until we release a Major one.

@DenisCarriere DenisCarriere changed the title Update distance radius according to Google's official earth radius Update distance measurement factors Mar 30, 2017
@morganherlocker
Copy link
Member

@morganherlocker Should we revert the PR & make a Turf v4.0 branch with the changes until we are ready to publish another major release.

@DenisCarriere We can publish a v4 release anytime. A major release does not need to be particularly "dramatic" -- it simply signals to a user that behavior may have changed in a way that could break dependent libraries. (not all libraries treat versioning this way, but turf has followed semver pretty strictly since v1.0.0 several years ago).

.... that said. I can also see a case made that this change simply "fixes" the distance module by making it more accurate and should therefore be considered a 0.0.x patch release. Thinking about this more deeply, I think either approach would actually be totally fine, as long as we communicate this in the changelog.

@DenisCarriere
Copy link
Member

DenisCarriere commented Mar 30, 2017

Ok! Sounds good, I'm thinking this should be a patch 0.0.x since the earth radius factor should of been 6378137 instead of 6373000.

This earth radius factor is the same as WGS84.
http://spatialreference.org/ref/epsg/wgs-84/html/

Also GDAL uses the same earth radius.
http://www.gdal.org/osr_tutorial.html

We can announce the patch change in the Issues.

@DenisCarriere
Copy link
Member

Published Major Release v4.0 to fix this issue.

@mourner
Copy link
Contributor

mourner commented Apr 5, 2017

@DenisCarriere @morganherlocker @dhivehi
I believe this is not correct. The Harvesine formula is meant to be used for spheres. Earth is a spheroid — it's not an ideal sphere. So radius varies depending on the point in Earth, and 6378km is the radius at the equator. Note that both links above (WGS84 and GDAL) use the word "spheroid" and have the value 298.257223563 in addition to the equator radius — this value is the inverse of ellipsoid flattening and defines the radius at the pole compared to the equator one.

The Harvesine approximation for Earth usually uses mean radius rather than equator radius, and mean radius equals 6371008.8 meters (which most formulas round to 6371km). So this patch actually made distances less precise on average.

@DenisCarriere
Copy link
Member

👍 @mourner Thanks for the enlightenment, what do you propose we do? I can't help revert it back if that's your recommendation. I'm also going to assume that all the other factors should also be reverted back?

@mourner
Copy link
Contributor

mourner commented Apr 5, 2017

@DenisCarriere I'd suggest reverting, or maybe changing to 6371008.8 (although the error compared to 6371000 is too tiny to be significant). See also great-circle distance article:

So long as we are assuming a spherical Earth, any single formula for distance on the Earth is only guaranteed correct within 0.5% (though we can do better if our formula is only intended to apply to a limited area).

A good choice[6] for the radius is the mean earth radius; in the limit of small flattening, this choice minimizes the mean square relative error in the estimates for distance.

@DenisCarriere
Copy link
Member

Closed due to #635 (comment)

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

No branches or pull requests

5 participants