Fix adding traffic signal penalties during compression #6419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Weight and duration penalties are flipped in the lambda function that applies penalties from traffic signals. Duration is in deciseconds, whilst weight is multipled by 10^weight_precision, with weight_precision being 1 by default. Therefore, for default routability profile, the penalties end up being the same, hence why no tests picked this up. If distance weight is used however, it will incorrectly apply an additional penalty to the weight, and not add the traffic signal delay to the duration in the routing graph. To confuse things further, in some API responses the values are correct because they use geometry data instead, but it's still possible that a sub-optimal route was selected. However, given the distance weight is in meters, and the additional penalty per traffic light would be 20, it's unlikely this would have changed the routing results. In any case, we correct the function to apply the arguments correctly.
mjjbell
force-pushed
the
mbell/edge_compressor_bug
branch
from
1d00b5c
to
932bdb6
Compare
SiarheiFedartsou
approved these changes
Oct 23, 2022
mattwigway
pushed a commit
to mattwigway/osrm-backend
that referenced
this pull request
Jul 20, 2023
…6419) Weight and duration penalties are flipped in the lambda function that applies penalties from traffic signals. Duration is in deciseconds, whilst weight is multipled by 10^weight_precision, with weight_precision being 1 by default. Therefore, for default routability profile, the penalties end up being the same, hence why no tests picked this up. If distance weight is used however, it will incorrectly apply an additional penalty to the weight, and not add the traffic signal delay to the duration in the routing graph. To confuse things further, in some API responses the values are correct because they use geometry data instead, but it's still possible that a sub-optimal route was selected. However, given the distance weight is in meters, and the additional penalty per traffic light would be 20, it's unlikely this would have changed the routing results. In any case, we correct the function to apply the arguments correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
As detected by @GitBenjamin in b17cbb4#r87212707
Weight and duration penalties are flipped in the lambda function that applies penalties from traffic signals.
Duration is in deciseconds, whilst weight is multiplied by
10^weight_precision, withweight_precisionbeing 1 by default.Therefore, for default routability profile, the penalties end up being the same, hence why no tests picked this up.
If distance weight is used however, it will incorrectly apply an additional penalty to the weight, and not add the traffic signal delay to the duration in the routing graph.
To confuse things further, in some API responses the values are correct because they use geometry data instead, but it's still possible that a sub-optimal route was selected.
However, given the distance weight is in meters, and the additional penalty per traffic light would be 20, it's unlikely this would have changed the routing results.
In any case, we correct the function to apply the arguments correctly.
Tasklist
Requirements / Relations
I will make a follow-up PR that implements #3601 to stop these types of mistakes, any maybe even detect some others in the codebase.