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 bug in thor_worker_t::build_trace on wrong edge_index assignment #4413

Merged
merged 6 commits into from
Dec 4, 2023
Merged

Fix bug in thor_worker_t::build_trace on wrong edge_index assignment #4413

merged 6 commits into from
Dec 4, 2023

Conversation

mahdihasnat
Copy link
Contributor

@mahdihasnat mahdihasnat commented Nov 26, 2023

Issue

#fixes #3633

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?

@nilsnolde
Copy link
Member

Your fork was 60 commits behind ;)

@mahdihasnat
Copy link
Contributor Author

mahdihasnat commented Nov 26, 2023

Yes, I tested it on top of release 3.4.0. I will merge this with master and push here.

Thanks you merged already

@nilsnolde
Copy link
Member

Ah ok. Always always test with master, never a release.

@mahdihasnat
Copy link
Contributor Author

I have tested current master with the around 2000 trace_attribute request. Didn't face the issue.

@nilsnolde
Copy link
Member

Great thanks for the feedback! Just needs a quick look by someone more familiar with map matching than me, I’m sure it’s fine:)

Could you do a quick changelog entry as well? At the bottom of the unreleased FIXED section would be great

@mahdihasnat
Copy link
Contributor Author

I have updated the changelog.
Thanks a lot.

@mahdihasnat
Copy link
Contributor Author

Hi @nilsnolde ,
I found another issue around edge_index assignment.
Lets say we have a path having edgeid = (0,0,0,1,1,2,3)
Current flow for edge_index calculation:

last_id = invalid id
edge_index = 0
for segment in paths
  // assign match_results array with current edge_index
  if (last_id != segment.edgeid)
    edge_index ++
  last_id = segment.edgeid

Following is the edge_index assigned for each segment:

edgeId 0 0 0 1 1 2 3
mismatch with previous edgeid at end of the loop 1 0 0 1 0 1 1
edge_index at beginning of the loop 0 1 1 1 2 2 3
correct edge_index at beginning of the loop 0 0 0 1 1 2 3

I propose the following logic for edge_index assignment:

last_id = invalid id
edge_index = 0
for segment in paths
  if (last_id != segment.edgeid && last_id.Is_Valid())
    edge_index ++
  // assign match_results array with current edge_index
  last_id = segment.edgeid

@mahdihasnat
Copy link
Contributor Author

Or even more performant logic:

last_id = invalid id
edge_index = -1
for segment in paths
  if (last_id != segment.edgeid)
    edge_index ++
  // assign match_results array with current edge_index
  last_id = segment.edgeid

@kevinkreiser
Copy link
Member

feel free to apply your other change as well and i can give you a review in the next few days

Copy link
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

this looks good to me, can you change that edge_index variable back to size_t and we can merge

@mahdihasnat
Copy link
Contributor Author

Yes, working on it

@nilsnolde nilsnolde merged commit 11c6358 into valhalla:master Dec 4, 2023
7 checks passed
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.

Wrong value for distance_along_edge for trace_attributes request
3 participants