-
Notifications
You must be signed in to change notification settings - Fork 347
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
Potential bug in calculations in metersToLongitudeDegrees #108
Comments
Any chance you've calculated the error ratio between using said values to see what % different values computed with one vs the other are? Are we talking off by millimeters, meters, kilometers...? |
It depends where on Earth you make the measurement -- the closer to the poles the bigger the mistake. The following shows dependency between the latitude and the difference in result between the two methods:
|
Ok, so it sounds like it's a pretty small difference. We use Geohashes under the hood, so behavior at the poles is pretty broken anyways (we also don't anticipate too many penguins or polar bears using Firebase ;) At normal latitudes (~15-30º), 5 bits of precision is probably good enough to place you within 2 meters, and that's pretty much within the error here (assume ~0.1% = 1m/km, which would be within the geohash area of a 5 character hash). Geohash precision listed below:
|
I do agree that the common geofire query -- "give me everything in N-km radius" -- is not expected to be extremely precise. However, then, the natural (while probably rhetorical) question is what on earth (no pun intended) the correction for the planet being a spheroid doing in the code in the first place? I may be wrong, but the following seems to suggest that somebody really wanted to fix something: // g_E2 == (g_EARTH_EQ_RADIUS^2-g_EARTH_POL_RADIUS^2)/(g_EARTH_EQ_RADIUS^2)
// The exact value is used here to avoid rounding errors
var g_E2 = 0.00669447819799; |
Version info
Firebase: 3.1.0
GeoFire: 4.1.1
I think there is a small bug in calculations in metersToLongitudeDegrees (or to be more precise in the value of g_E2 this function is using):
These are the relevant snippets of code from geofire.js:
As part of its calculation metersToLongitudeDegrees finds deltaDeg, which is the length of a one-degree arc of a parallel at the given latitude.
deltaDeg is calculated as:
R, the radius of the same parallel, should be equal to deltaDeg * 360 / (2 * PI), i.e.
Would the Earth be a perfect sphere with radius of g_EARTH_EQ_RADIUS, then the radius at a particular parallel would be:
However, because the polar radius of Earth is smaller than the equatorial it should be R < R_sphere.
The issue is that the calculation of R above (i.e. the one used by metersToLongitudeDegrees) renders R > R_sphere (for 1 > g_E2 > 0).
The following is my attempt to understand what is going on:
If each meridian is an ellipse with semi-axes equal to Req and Rpol then:
R2 / Req2 + R2 * tan2(latitude) / Rpol2 = 1
From here we get:
R2 * (1 / Req2 + tan2(latitude) / Rpol2) = 1
R = 1 / sqrt (1 / Req2 + tan2(latitude) / Rpol2)
R = Req / sqrt (1 + Req2 * tan2(latitude) / Rpol2)
R = Req / sqrt (1 + Req2 * sin2(latitude) / (Rpol2 * cos2(latitude)))
R = Req * cos(latitude) / sqrt (cos2(latitude) + Req2 * sin2(latitude) / Rpol2)
R = Req * cos(latitude) / sqrt (1 - sin2(latitude) + Req2 * sin2(latitude) / Rpol2)
R = Req * cos(latitude) / sqrt (1 - sin2(latitude) * (1 - Req2 / Rpol2))
R = Req * cos(latitude) / sqrt (1 - sin2(latitude) * ( (Rpol2 - Req2) / Rpol2))
g_E2_2 = (Rpol2 - Req2) / Rpol2
R = cos(latitude) * Req / sqrt(1 - g_E2_2 * sin2(latitude))
This is very close to what metersToLongitudeDegrees is using, but in g_E2_2 the equatorial and polar radii are flipped compared to g_E2.
The calculation of g_E2_2 gives about -0.00673950125, which is a negative value and as such solves the issue described above.
The text was updated successfully, but these errors were encountered: