-
Notifications
You must be signed in to change notification settings - Fork 499
Use floor when converting degrees to integer #717
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
Comments
I wrote (in golang) some code that for a billion latitudes, computed the integer value using both round and floor methods, and then converted the integer value back and checked if the original latitude is contained by that range.
So I think the implementations need to be changed to use floor, not round. |
Also - the Go implementation has untyped values for many of the constants and they should change to be explicitly typed. |
Just in case you wanted more bad rounding news, Python uses yet another approach to rounding called "round ties to even", which is, umm, unexpected: >>> round(3.5)
4
>>> round(4.5)
4 |
Based on tests it appears that it should actually be a |
Ah ha. For latitude, we can use UPDATE: I tried and it looks like the two approaches are the same. |
The encoding and decoding implementations use integers rather than floats to avoid the build up of precision losses through repeated division or multiplication, and to improve the computation speed.
The initial conversion of degrees to integer for latitude is:
(
finalLatPrecision
is 2.5e7, and is the number of divisions in one degree of latitude that a maximum length OLC code can represent.)The conversion results in an integer value that will be used to compute the code. Conversion from the integer to the code and back is stable (I mean there are no issues with precision loss).
Ideally we would have an integer that when converted back to a range contains the original latitude value. By range, I mean using the conversion value and adding
1/finalLatPrecision
.Unfortunately using
math.Round()
means that the resulting integer can be 1 higher than it should be, and after converting back to a range it does not contain the original latitude.(Everything also applies to longitude.)
The impact is small because we are talking about being out by 1/2.5e7 or 1/8.192e6 degrees, or about half a centimetre, but this won't be helping the test cases.
The text was updated successfully, but these errors were encountered: