Add epsilon option to booleanPointOnLine

[okcoker]

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.

I'm working on an algorithm that uses a few of the modules here. I used turf.lineIntersect to find intersection points of two polygons. Naturally when checking which line of the polygon one of the points was on, I was surprised to find out booleanPointOnLine was returning false for all of the line strings of the polygon. I even used pointToLineDistance and that function was returning 0 as expected. Digging deeper into why, I noticed the cross product within booleanPointOnLine was expected to be 0. However when using lat/lng coordinates, the cross product ended up being something super small (ie 4e-17) which is basically 0 but obviously not === 0. None of the tests contained numbers with the precision of lat/lng coordinates so this PR does just that.

I've decided to give precision a default value because I think this function should have returned true in the first place for me. Even though I've made the default value 5 here (Updated this to 10 for even more confidence with lng/lat points), it could really be anything else (for my example less than 17 would suffice). As far as this function goes, a number that starts with 10 zeros could be considered 0 for most cases I think.

[mfedderly]

I'm a little bit concerned about just adding an epsilon here without really understanding the relationship between the cross product value and the accuracy in real terms. It looks like PostGIS actually takes a distance in meters instead of the epsilon or precision approach.

Additionally I'm a little bit concerned that this change also does not correctly impact the ignoreEndVerticies option.

Thoughts?

[okcoker]

I'm a little bit concerned about just adding an epsilon here without really understanding the relationship between the cross product value and the accuracy in real terms. It looks like PostGIS actually takes a distance in meters instead of the epsilon or precision approach.

Are you speaking of ST_DWithin? I have very limited experience with PostGIS so I'm not sure what what the best approach would be here in terms of making the function signature here similar. I feel adding a parameter like this (precision/epsilon) gives the most control to the consumer of this function to do what they'd like and ideally you wouldn't even have to touch this parameter as it would Just Work™.

I think the only reason I even ran into this problem is because all of the test cases within this package were using simple integers instead of of lng/lat points.

Additionally I'm a little bit concerned that this change also does not correctly impact the ignoreEndVerticies option.

Yes I definitely skipped that bit of the function for my use case :P Let me explore that side a bit more and add some more test cases

[mfedderly]

Yeah I spent some time today looking into ST_DWithin and that's where I got the idea about using distance here. I think the problem with the epsilon/precision approach is that you're using it here on the cross product. I don't actually know what units the cross product is in, so it is hard to reason about the what that value means for the potential error allowed. Its also a little bit awkward to change the behavior of this method in a way that isn't exactly backwards compatible, even though I agree that this probably needs to work in the way you're suggesting.

[okcoker]

Yeah I spent some time today looking into ST_DWithin and that's where I got the idea about using distance here.

So I found ST_DWithin when googling for a point-on-line solution but I think this turf module actually is meant to be closer to ST_Intersects, while ST_DWithin is probably closer to boolean-point-in-polygon. It seems ST_Intersects uses a constant 0.00001 meters for tolerance but both functions use meters as the default so is there any reason not to assume that for this function?

I think the problem with the epsilon/precision approach is that you're using it here on the cross product. I don't actually know what units the cross product is in, so it is hard to reason about the what that value means for the potential error allowed.

Its also a little bit awkward to change the behavior of this method in a way that isn't exactly backwards compatible, even though I agree that this probably needs to work in the way you're suggesting.

Yeah definitely. It feels as though one would have to sort of guess what value they need for their specific use case. As mentioned above, is there any reason not to assume meters for this? One way to go about this is to default the precision value to that of the input. For example, a point like [1,0] would work as the module does now where there's no room for error (cross === 0). But a point like [-75.2580499870244, 40.00180204907801] would use an epsilon value of 10^x where x is the number of decimal places. That being said, giving a default epsilon value for extra confidence would work in both cases and remove the need for converting to a string and counting decimal places.

In terms of backwards compatibility you're right in that this may "break" existing implementations. Originally I did have the undefined check to pass null instead of a default 10 but in order to keep backwards compatibility, I think it would be necessary to put that back in. I will also rename precision to tolerance to be closer to ST_Intersect. Having read more about floating points with this sort of check, I think epsilon is appropriate.

Other than that, I will make these changes now. I'm also open to other suggestions here as I think there's room for improvement and I don't want to have to keep using a custom fork 😛

[okcoker]

Ok so I've opted for the more technical epsilon as a parameter. When dealing with lng/lat points, one will most likely have to use this parameter and pass in their own threshold value (ie { epsilon: 1e-10 }) in order to get the results they probably expect. I've also make this backwards compatible by not even considering it unless it is explicitly passed as an option. Added an additional test as well.

[mfedderly]

Sweet, thanks for making these changes!