Reject alternatives with too long detours #3238
Conversation
| | | | ||
| E-F | ||
| )"; |
There was a problem hiding this comment.
typical example of an alternative that should be rejected by these changes
| @@ -15,7 +15,13 @@ using namespace valhalla::midgard; | |||
| */ | |||
| namespace { | |||
| // Defaults thresholds | |||
| // TODO: global stretch parameter should depend on the optimal route cost/duration. | |||
There was a problem hiding this comment.
I'm sure that we miss some good alternatives for short routes because of one global threshold. Added as "TODO", but it should be transformed into an issue/PR
There was a problem hiding this comment.
and, imho, we should also refactor max_sharing (https://github.com/valhalla/valhalla/blob/master/src/thor/alternates.cc#L25-L47) coefficient in the following way:
- a little bit increase
max_sharingcoefficient for shorter routes. I think value0.6for routes < 20km is too strict, it should be ~0.7. - replace piecewise constant function with a smoother one (cubic?) to ensure that for two close routes
max_sharingconstant values also will be close. It's a little bit strange that for 9.9-km route we will usemax_sharingconstant equals to0.5but for 10-km route this constant will be0.6.
There was a problem hiding this comment.
both of these suggestions make way more sense to me as well 👍
No good alternatives were found
Other alternatives were proposed
|
src/thor/alternates.cc
Outdated
| const auto& idxs_opt = diff_segment.first; | ||
| const auto& idxs_alt = diff_segment.second; |
There was a problem hiding this comment.
this naming is difficult for my brain. idxs_opt is the segment of the optimal path which is different and idxs_alt is the segment of the candidate path that is different. what about like unique_optimal_segment and unique_candidate_segment? maybe also diff_segment would be like unique_segments? i dont know just a suggestion, though pretty long names...
There was a problem hiding this comment.
I like that, thank you !
src/thor/alternates.cc
Outdated
| const auto& idxs_opt = diff_segment.first; | ||
| const auto& idxs_alt = diff_segment.second; | ||
|
|
||
| if (idxs_opt.first == optimal_path.size()) { |
There was a problem hiding this comment.
we could use a comment here i think, its hard to understand that this is really the second clause of the comment in the next if block. basically its saying, there was no start to a unique part in the optimal path, as all its edges were found in the alternate path. then the second if is really saying there was a unique part in the alternate path past the end of the optimal, which is just wasteful so reject it
There was a problem hiding this comment.
by the way is there any way to make that happen in gurka? im guessing its pretty difficult. you have to have two loki::search candidates on both locations so that the search paths can go the "wrong" way and then meet after the "right" way finds the optimal route
There was a problem hiding this comment.
I've met such corner case when tested this PR; but it's difficult to reproduce
There was a problem hiding this comment.
no worries, just wondered if you tried
Issue
A motivation of this PR is to reject bad alternatives that have unreasonable long detours comparing to the optimal route.
Code is simple enough: find a different segment for two routes and compare their costs. In case cost of alternative segment is much bigger than cost of the optimal segment -> reject such alternative.
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?