Improve rerouting logic if all candidates are very different from the initial route #3664
Conversation
…from the original route
| expectedTravelTime: 0, | ||
| typicalTravelTime: nil, | ||
| profileIdentifier: .automobile) | ||
| super.init(legs: [leg], shape: nil, distance: 0, expectedTravelTime: 0, typicalTravelTime: nil) |
There was a problem hiding this comment.
Little hack, because description property cannot be overriden
There was a problem hiding this comment.
This is correct, since Route.description is based on RouteLeg.name:
https://github.com/mapbox/mapbox-directions-swift/blob/28a0bc98ca13ac7cbfa08d06f273dcd94503d696/Sources/MapboxDirections/DirectionsResult.swift#L191
https://github.com/mapbox/mapbox-directions-swift/blob/28a0bc98ca13ac7cbfa08d06f273dcd94503d696/Sources/MapboxDirections/RouteLeg.swift#L353
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
|
|
||
|
|
| // If the most similar route is still more than 50% different from the original route, | ||
| // we fallback to the fastest route which index is 0. | ||
| guard bestCandidate.element.route.description.count > 0 else { return 0 } | ||
| let similarityScore = Double(bestCandidate.element.editDistance) / Double(bestCandidate.element.route.description.count) |
There was a problem hiding this comment.
bestCandidate.element.route.description or target?
There was a problem hiding this comment.
Good question, I don’t have a good intuition for this. Can we try out a few scenarios to see which would be more correct?
There was a problem hiding this comment.
If the best candidate is when we remove letters from the target, then we might get more than 1 here. I.e.
target = "AAAAAA"
bestCandidate = "AA"
editDistance = 4 // remove any 4 letters
score = 4 / len("AA") = 4 / 2 = 2
At the same time, if the best candidate is when you add letters, then we can get the same "larger than 1" if compared to the target length.
We can divide by the sum of the sizes to guarantee that we get the value within 0...1, but it's a bit more difficult to interpret such a score, i.e.
"AAAAAA" -> "AAAAAA" = 0 / 12 = 0.00
"AAAAAA" -> "AAAAA" = 1 / 11 = 0.09
"AAAAAA" -> "AAAA" = 2 / 10 = 0.20
"AAAAAA" -> "AAA" = 3 / 9 = 0.33
"AAAAAA" -> "AA" = 4 / 8 = 0.50
"AAAAAA" -> "A" = 5 / 7 = 0.71
"AAAAAA" -> "" = 6 / 6 = 1.00
"AAAAAA" -> "AAAABB" = 2 / 12 = 0.17
"AAAAAA" -> "AAABBB" = 3 / 12 = 0.25
"AAAAAA" -> "AABBBB" = 4 / 12 = 0.33
"AAAAAA" -> "BBBBBB" = 6 / 12 = 0.50
"AAAAAA" -> "BBB" = 6 / 9 = 0.6
"AAAAAA" -> "AAAAAAC" = 1 / 13 = 0.08
"AAAAAA" -> "AAAAAACCCC" = 4 / 16 = 0.25
"AAAAAA" -> "AAAAAACCCCC" = 5 / 17 = 0.29
Maybe we can compare the score to 0.25 as for "replacing the half of the string" or similar.
There was a problem hiding this comment.
I added some tests for different candidate lengths and it seems like @bamx23's algorithm might show better results for some edge cases.
It's now a little bit harder to understand, but I added a comment with a link to this explanation.
Even if we are unable to select the most common route we will fallback to the fastest route which not as bad.
| }).enumerated().min(by: { $0.element.editDistance < $1.element.editDistance }) else { return nil } | ||
|
|
||
|
|
||
| // If the most similar route is still more than 50% different from the original route, |
There was a problem hiding this comment.
This number came from #3597 (comment). There are only ever two road names in the route description per leg, so a 50% match is roughly one of the two names matching (modulo any differences in name length or minor things like “South”). We could probably lower the threshold without much problem, but this becomes a bugfarm, then the per-step approach in mapbox/mapbox-navigation-android#3116 (comment) would be a decent plan B.
| }).enumerated().min(by: { $0.element.editDistance < $1.element.editDistance }) else { return nil } | ||
|
|
||
|
|
||
| // If the most similar route is still more than 50% different from the original route, |
There was a problem hiding this comment.
This number came from #3597 (comment). There are only ever two road names in the route description per leg, so a 50% match is roughly one of the two names matching (modulo any differences in name length or minor things like “South”). We could probably lower the threshold without much problem, but this becomes a bugfarm, then the per-step approach in mapbox/mapbox-navigation-android#3116 (comment) would be a decent plan B.
| // If the most similar route is still more than 50% different from the original route, | ||
| // we fallback to the fastest route which index is 0. | ||
| guard bestCandidate.element.route.description.count > 0 else { return 0 } | ||
| let similarityScore = Double(bestCandidate.element.editDistance) / Double(bestCandidate.element.route.description.count) |
There was a problem hiding this comment.
Good question, I don’t have a good intuition for this. Can we try out a few scenarios to see which would be more correct?
| expectedTravelTime: 0, | ||
| typicalTravelTime: nil, | ||
| profileIdentifier: .automobile) | ||
| super.init(legs: [leg], shape: nil, distance: 0, expectedTravelTime: 0, typicalTravelTime: nil) |
There was a problem hiding this comment.
This is correct, since Route.description is based on RouteLeg.name:
https://github.com/mapbox/mapbox-directions-swift/blob/28a0bc98ca13ac7cbfa08d06f273dcd94503d696/Sources/MapboxDirections/DirectionsResult.swift#L191
https://github.com/mapbox/mapbox-directions-swift/blob/28a0bc98ca13ac7cbfa08d06f273dcd94503d696/Sources/MapboxDirections/RouteLeg.swift#L353
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
azarovalex
force-pushed
the
azarovalex/improve-reroute-logic
branch
from
53878e7 to
28efaa9
Compare
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
# Conflicts: # CHANGELOG.md
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
|
No breaking changes detected in MapboxCoreNavigation |
|
No breaking changes detected in MapboxNavigation |
Fixes #3597