Skip to content

Conversation

@Themacprod
Copy link

Hi,

  • index.js : added the earth radius in the distance computation.
  • test.js : fixed unit test + adding new tests

Themacprod added 3 commits May 4, 2015 23:10
Hi,

I already sent a response to an issue with turf-buffer with a JSFiddle to show the issue. I modified this JSFiddle to show the distance computation error while using turf-distance.

http://jsfiddle.net/Themacprod/r2fp5t66/1/

This proposal can be improve by using the latitude of the middle of the two coodinates.
The "R" value was not used.
@jchristin
Copy link

Dear Turf Team,

Could you give us some inputs for this pull request? I am also interested in the fix.

Thanks,
Julien

@morganherlocker
Copy link
Member

Sorry for the delay. This PR got lost in the shuffle. I'll take a look at this tomorrow.

@morganherlocker
Copy link
Member

Ok, looking things over, my only concern is the performance drop. I'm going to tinker with it a bit and see if it can be made faster.

old

turf-distance x 4,941,766 ops/sec ±1.03% (93 runs sampled)

new

turf-distance x 3,596,891 ops/sec ±1.08% (92 runs sampled)

@morganherlocker
Copy link
Member

Ok, I'm a bit confused here. Can you lay out a specific test case that was failing, that is now fixed? This method may be better, but I'm having to dig through a lot to determine what the relevant changes are. The example you linked to has a lot going on (buffers, points, etc. which are not related).

Basically just a test case with a couple screenshots is all I need to move forward.

@Themacprod
Copy link
Author

Hi,

I think you should do your tests at a good distance from Ecuador, it will show you the impact of the latitude in the distance computation.

I've done a small jsfiddle to explain the issue (http://jsfiddle.net/Themacprod/r2fp5t66/3/)

It is easier to explain by drawing a turf-buffer (red buffer) :
image

If you trace a non-eleptical circle (green buffer) , you should have a buffer like that (I put it above the turf-buffer circle)
image

I placed 3 points :

image

Here are my results
Distance from #1 (turf) = 0.113 km | Me = 0.4 km
Distance from #2 (turf) = 0.572 km | Me = 0.4 km
Distance from #3 (turf) = 0.405 km | Me = 0.283 km

Here are the Google result for the distance :
image

image

@morganherlocker
Copy link
Member

Ok, the issue I'm having understanding your suggestion here is that turf-buffer does not use turf-distance in any way. The fact that buffers are elliptical is completely unrelated; that is due to the fact that JSTS uses planar geometry calculations instead of geodesic (something I am working on fixing in turf-buffer). Comparing the two buffers here, does not show an issue with turf-distance. turf-destination also uses geodesic calculations. Run turf-destination around a point, and you will see a nice perfect circle.

turf-distance already employs geodesic calculations. Basically what I need is a specific case where the the distance between 2 points is correct in some other program, but incorrect in turf-distance.

@Themacprod
Copy link
Author

Here is a more explicit sample:

Marker #1 = [45.535017 , -73.645906 ]
Marker #2 = [45.53835 , -73.586447 ]

According to Google the distance between those 2 points is 4.65 km :
image

var Lat1 = 45.535017,
Lng1 = -73.645906,
Lat2 = 45.53835,
Lng2 = -73.586447;

var turfpoint1 = turfpoint([Lat1, Lng1], {name: 'Location 1'});
var turfpoint2 = turfpoint([Lat2, Lng2], {name: 'Location 2'});

var distanceturf1 = turfdistance(turfpoint1, turfpoint2, "kilometers");

console.log('Distance = ' + Math.round(distanceturf1 * 1000) / 1000 + ' km');

Distance = 6.614 km

With my changes I get 4.635 km (Simplified jsfiddle :http://jsfiddle.net/rq0b0gfb/2/)

@Themacprod
Copy link
Author

Ok sorry guy, I feel a little bit stupid, unfortunatly I swap latitude and logitude in the turf point....
Forget this pull-request

@Themacprod Themacprod closed this Jun 11, 2015
@morganherlocker
Copy link
Member

@Themacprod no worries at all. This stuff is really difficult to wrap one's head around (for me too). Thank you so much for the effort and following up with the tests/screenshots/examples/explanations/etc.

Seriously, investigations like this are all a part of the process, and hugely beneficial even when a PR does not ultimately get merged.

@nevi-me
Copy link

nevi-me commented Jun 11, 2015

If I'm not too late; @morganherlocker what would be the ideal case in @Themacprod's last example? Should we get the same distance as what Google (and few others) get? Because that would be a matter of changing R to the value in https://github.com/Turfjs/turf-distance/issues/4.

I saw this PR last night but couldn't understand it, so thought I'd look at it this morning. The red flag for me was @Themacprod not substituting the current R with a different value, but calculating it each time, but glad that's resolved 👍

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.

4 participants