Skip to content
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

Fix roundoff issue in Tiles #4585

Merged
merged 4 commits into from
Feb 15, 2024
Merged

Fix roundoff issue in Tiles #4585

merged 4 commits into from
Feb 15, 2024

Conversation

dnesbitt61
Copy link
Member

@dnesbitt61 dnesbitt61 commented Feb 14, 2024

Issue

#fixes #4584
Using float in the Tiles Row and Col methods can lead to an incorrect tile id being computed (along with incorrect base lat, lng). Using double as the argument to these methods fixes the issue.
For discussion - will this change cause any compatibility issues?

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

Row and Col methods) where an incorrect tile Id is computed (along with
incorrect Base lat,lng from that tile).
to rare roundoff issues where a neighboring tile is returned.
@gknisely
Copy link
Member

@dnesbitt61 need to update the changelog

@kevinkreiser
Copy link
Member

im trying to reason about your comment on data compatibilty. old code would expect that a node would land in a different tile than new code. which means old data built with old code would have certain nodes land in the wrong tiles. this would be a problem if we used this method in a scenario where:

  1. we had old code
  2. we had new data
  3. loki used it to find the right tile to look for candidate edges
  4. but the n ew data wou ld have the edges in a different tile

the good news is this doesnt really matter because loki never stops looking in one place it looks at closest first and next closes next and so on. the only risk is that in this scenario it finds something that isnt the actual closest thing, which does suck but its not catastrophic. to me i think we can ship this without worrying about compatibility. the data wont crash anything it will at worst give sub par results for these specific edge cases

@dnesbitt61
Copy link
Member Author

how can codecov fail! I added a unit test. Oh well

@nilsnolde
Copy link
Member

🎲 🎲

Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch again thanks!

@nilsnolde nilsnolde merged commit efff65d into master Feb 15, 2024
8 of 9 checks passed
@nilsnolde nilsnolde deleted the tiles_float_fix branch February 15, 2024 12:00
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.

Roundoff issue within Tiles
4 participants