SloppyMath.asin says it has an error around 1E-7, which translates to an error of 1.3m in haversine if I'm not mistaken. I don't think this is an issue, but this makes me wonder whether ARC distance computation should fall back to PLANE on short distances (to minimize the error) or maybe if we should have both a precise ARC and a sloppy SLOPPY_ARC GeoDistances (probably with SLOPPY_ARC as the default since I think the trade-off is better)?

I agree with @jpountz I think we should fall back if we have short distances. Generally when we port stuff from Lucene as an XClass we don't port the tests. I think in this case it's ok but we should also put a check for 4.6 in the XSloppyMath class itself. We usually do this as a static block like:

static {
  assert Version.CURRENT.luceneVersion.compareTo(org.apache.lucene.util.Version.LUCENE_45) == 0 : "Remove XSloppyMath once 4.6 is out";
}

You're right. I worked the whole day on getting the error out. I hope you agree that the error doesn't matter for great distances.

Yeah for great distances this really doesn't matter. We might want to think about making it configurable in the mapping such that you can opt out entirely?

@s1monw what do you want to make configurable in the mapping?
In the latest version I managed to reduce the error less 1.0E-4 meters in haversin. But I need to calculate the plane distance every time and branch on the result. Another problem I got is a different value of the earth major axis used in the Lucene implementation. I left a comment on jira to move this to the signature of the haversin method. If robert agrees, we are able to calculate not only with WGS84 compatible distances but also use a more accurate unit (mm) to calculate the distance and make the results more accurate this way.
The current version is a little buggy and I need to find out, if my accurate version of haversin is really accurate.

sorry @chilling I think mapping is not the right place for it. I really just wanted to make sure that people can opt out of the sloppy calculation so we maybe expose it as a query param "sloppy" : [true|false|"xmiles"|"xmeters"] something like that?

@s1monw I see. This is actually a good idea.

Florian, thanks for the updates. I think it looks good, but it would be easier for me to review if you could rebase against master (so that we don't need XSloppyMath anymore) and squash the commits (since they all apply to the same lines of code). Thanks!

hmm seems like the latest push still has XSloppyMath...

@jpountz thanks for your review, I'm going to set this things up tomorrow. @s1monw for this update I let in there, how about backporting?

@chilling I don't think there is an issue with backporting? (the 0.90 branch is on Lucene 4.6 as well)

I disabled the fallback to plane and all accuracy tests still passed. I need to spend some more time to understand when we can expect the highest errors and if we actually need this fallback. If you have already investigated it, please share. :-)

I've been looking into this a bit further and I don't think we need the fallback on short distances: the approximate functions use taylor series at 0 and are thus very precise on small data (and short distances mean that cos and arcsin have been computed against small data).

So I would vote to just remove the fallback to plane and change the tests to make sure the delta is within 1‰ instead of 1mm (I just patched the test case to make sure it passes with such a test).

+1 to squash and push

For the record, GeoDistanceSearchBenchmark seems to be very happy with this change. It reports times which are 3.2x to 4.5x faster as with accurate arc distance computations.