Penalize closed edges when routing through them #2964

mandeepsandhu · 2021-03-24T01:57:34Z

Penalize closed edges by using lowest possible speed for the costing
model in use. This ensures we exit out of closures asap.

mandeepsandhu · 2021-03-24T17:00:25Z

Note to reviewers: Looking for feedback on 2 things:

For all costing models except motor_scooter, I'm assuming a min speed of 5kph. I mostly picked it as it was already defined as the minimum reliable live speed value. Is this too fast for a closure (closures have live speed of 0, so that way anything higher than that is too fast 😛 ). The purpose of penalizing the edge is to make the route leave a closure asap, and also to give a more accurate ETA.
For motor_scooter, 5kph seemed not slow enough, so I used 2kph instead. Should it be made more slower?

kevinkreiser · 2021-03-24T17:48:49Z

@mandeepsandhu i feel like 5kph is slow enough even for motor_scooter, this seems like a fittingly horrible penality, after all, the walking speed we choose is faster than this!

mandeepsandhu · 2021-03-24T18:02:49Z

@mandeepsandhu i feel like 5kph is slow enough even for motor_scooter, this seems like a fittingly horrible penality, after all, the walking speed we choose is faster than this!

@kevinkreiser @purew pointed out that slowest live traffic speed that we store in our tiles is 2 kph. So technically closure's speed should be slower than that (i can imagine an edge case where a closure is right next to really slow moving traffic of 2kph and the router selects the closed road since it appears faster).

Should I just use 1kph for all costings?

kevinkreiser · 2021-03-24T18:11:50Z

that is true... but the question is more about what kind of values does one actually see, not what is the smallest value that you can see. ill just mention that the higher the penalty the more searching we will have to do if that is the only way to get to the location. to me it seems you want to discourage using them, its not about relative to other bsaically closed edges, its about with respect to other not closed edges. anyway 1kph will definitely work, it just seems a bit overkill. i dont feel strongly about it!

mandeepsandhu · 2021-03-24T20:44:49Z

that is true... but the question is more about what kind of values does one actually see, not what is the smallest value that you can see. ill just mention that the higher the penalty the more searching we will have to do if that is the only way to get to the location. to me it seems you want to discourage using them, its not about relative to other bsaically closed edges, its about with respect to other not closed edges. anyway 1kph will definitely work, it just seems a bit overkill. i dont feel strongly about it!

I think you raise a good point about very low speeds extending search from the other direction causing slower search convergence. I'll use 5kph even for motorscooter. While this value is not a very accurate reflection of reality, it serves the main purpose of not staying within closures for more than necessary, giving decent ETAs while not slowing down route response unreasonably. I'll update the PR this change.

mandeepsandhu · 2021-03-24T22:10:14Z

Change the closure speed to be 5kph for all costing models. This did change the scooter routes through closures a bit (thats why in test/gurka/closure_penalty I had to have a different expected route for motor_scooter) likely because the difference in cost between closed and open road is not that significant.

CC: @kevinkreiser @purew

danpat · 2021-03-24T22:24:14Z

@mandeepsandhu would it possibly make more sense to leave the speed alone (or fallback to one of the layers below live speed) and just increase the cost.cost value, not cost.secs ?

struct Cost {
  float cost;
  float secs;
...
}

Pathfinding is done base on the .cost value, not the .secs value ultimately....

mandeepsandhu · 2021-03-24T23:04:25Z

@mandeepsandhu would it possibly make more sense to leave the speed alone (or fallback to one of the layers below live speed) and just increase the cost.cost value, not cost.secs ?

The speed value is only changed for calculating the edge cost. The value returned from GetSpeed is not changed, and it already ignores speed for closed edges and falls back to using other sources for speed.

I could change just cost.cost, but not sure why we'd want to do that? If we don't change cost.secs the ETA calculated would be the same as if the edge was not closed. Is that the intention, i.e avoid closed edges (by increasing cost) for routing, but don't change the route duration when taking closed edges?

struct Cost {
  float cost;
  float secs;
...
}
Pathfinding is done base on the .cost value, not the .secs value ultimately....

danpat · 2021-03-24T23:44:10Z

Is that the intention, i.e avoid closed edges (by increasing cost) for routing, but don't change the route duration when taking closed edges?

@mandeepsandhu yeah, that was my intention with this suggestion. My concern with adjusting the speed is that if you're starting a route on a potentially long piece of highway that's marked closed (let's say, for the next couple of miles), then the ETA will have potentially large amount added to it. However, the fact that you're on the road is a hint that maybe the closure data is incorrect. I agree getting off the closure ASAP should be a priority, but the I think we have less information about what the actual travel speed should be:

https://github.com/valhalla/valhalla/blob/master/valhalla/baldr/traffictile.h#L71-L89

by definition, we define closures as "live traffic speed is 0km/h" which we obviously can't use. There's a fight here between asserting that "yes, I'm actually on this closed road and driving", and the data saying "that road is closed, as indicated by this speed of zero".

I think a happy middle ground here would be to fallback to the next level of speed data we have available, but getting off the road ASAP - that can be achieved by tweaking the .cost, but not the .secs.

kevinkreiser · 2021-03-24T23:53:14Z

Yeah this is a case where unfortunately there is no right answer as Dan is pointing out. You say you are sure you are on a closed road, we say we are sure its closed, what the heck is the speed!? The conservative approach of only penalizing and not modifying the cost.secs does sound like a good first stab. Whether it would be more accurate if we lowered the speed on the closed sections is sadly a very difficult to test hypothesis

mandeepsandhu · 2021-03-25T03:11:24Z

Yeah this is a case where unfortunately there is no right answer as Dan is pointing out. You say you are sure you are on a closed road, we say we are sure its closed, what the heck is the speed!? The conservative approach of only penalizing and not modifying the cost.secs does sound like a good first stab. Whether it would be more accurate if we lowered the speed on the closed sections is sadly a very difficult to test hypothesis

Yeah its interesting that we're trying to put a guesstimate on the speed of a 0-speed stretch of the road. Wonder if in such scenarios we should be giving a fixed time penalty to the route duration. Eg if we know traffic closures (maybe for a particular area) typically clears up within 10 or 30 mins (I'm just pulling random numbers), we could add it to the ETA (while still putting high cost for traversal of a closed edge).

Anyway let me take a stab at what @danpat suggested, i.e only change the cost and not duration for a closed edge.

mandeepsandhu · 2021-03-25T05:16:10Z

@danpat @kevinkreiser I'm now evaluating only the cost component with reduced speed and the duration still uses the normal edge speed. Have a look and let me if you have further comments.

I've also done perf test to make sure we didn't regress due to added calculation of duration used for costing. The perf matches that of master.

danpat · 2021-03-25T19:46:42Z

@mandeepsandhu 👍 looks good - want to clean up the history, this feels like it should be a single commit.

purew · 2021-03-25T19:47:09Z

@mandeepsandhu +1 looks good - want to clean up the history, this feels like it should be a single commit.

This repository only allows squash merges.

mandeepsandhu · 2021-03-25T19:47:44Z

@mandeepsandhu +1 looks good - want to clean up the history, this feels like it should be a single commit.

This repository only allows squash merges.

☝️

kevinkreiser · 2021-03-25T20:59:17Z

Yep no need for rebasing. We force you to squash merge exactly because it keeps the history nice. Excellent feature from github for sure!

kevinkreiser · 2021-03-26T17:22:16Z

src/sif/autocost.cc

+  // Use reduced speed for costing purposes, and normal edge speed for calculating duration (sec).
+  // This means closed edges cost more, but have the same traversal time as if they were open
+  auto speed_for_costing = IsClosed(edge, tile) ? kMinSpeedKph : final_speed;
+
+  float sec = (edge->length() * speedfactor_[final_speed]);
+  float cost_duration = (edge->length() * speedfactor_[speed_for_costing]);


another approach we have taken here is to add a fixed penalty as a multiplier (factor) to any closed edge. rather than computing another speed, you just multiply it. and we've also made these request parameters so we can tune them if we need to rather than hardcoding a single penalty. another option is having a length of time penalty which is added to the final value and also configurable at request time. i personally think either of these options is a little cleaner than calculating a duration based on a sepcific speed but i dont feel strongly about it other than it being so different than the existing methods for penalization

I've switched to using a factor thats multiplied to the final cost if traversing a closed edge. I derived the value comparing how high the cost was using the previous implementation of using reduced speed for costing.

CC: @danpat

kevinkreiser · 2021-03-29T13:21:55Z

valhalla/sif/dynamiccost.h

@@ -878,6 +879,9 @@ class DynamicCost {
    alley_penalty_ = costing_options.alley_penalty();
    destination_only_penalty_ = costing_options.destination_only_penalty();
    maneuver_penalty_ = costing_options.maneuver_penalty();
+    // TODO: Get this from the costing model


adding this is quite straightforward:

just add the float to the protobuf costing options defintion

set it by parsing the value here: https://github.com/valhalla/valhalla/blob/master/src/sif/dynamiccost.cc#L275, defaulting to your value of 9.f if not specified

then make sure to initialize it in the constructor: https://github.com/valhalla/valhalla/blob/master/src/sif/dynamiccost.cc#L82

document it and mention its only for motorized vehicles?

other than this ^^ pr looks good!

Done. Its now a costing option in the request. Value ranges from 1. to 10, with 1 meaning don't apply an additional penalty to closed edges. I've also limited the max value to 10 (defaults to 9) so that we don't extend the search for too long from one end.

CC: @kevinkreiser @danpat

Penalize closed edges by using lowest possible speed for the costing model in use. This ensures we exit out of closures asap.

…d edge

kevinkreiser

LGTM when CI is finished

mandeepsandhu requested review from kevinkreiser, gknisely, danpat and purew March 24, 2021 01:57

mandeepsandhu force-pushed the closure_recosting branch from 671c790 to bbbb019 Compare March 24, 2021 01:58

mandeepsandhu force-pushed the closure_recosting branch from 76f11a7 to 4db8e6e Compare March 24, 2021 21:29

mandeepsandhu force-pushed the closure_recosting branch from 3e4b296 to f7ba8b3 Compare March 25, 2021 17:29

danpat previously approved these changes Mar 26, 2021

View reviewed changes

kevinkreiser reviewed Mar 26, 2021

View reviewed changes

mandeepsandhu dismissed danpat’s stale review via 4a4683e March 26, 2021 23:47

kevinkreiser reviewed Mar 29, 2021

View reviewed changes

mandeepsandhu added 3 commits March 30, 2021 09:53

Penalize closed edges when routing through them

dc00411

Penalize closed edges by using lowest possible speed for the costing model in use. This ensures we exit out of closures asap.

update changelog with R number

f02f99b

lint fix

75a8dd9

mandeepsandhu added 12 commits March 30, 2021 09:53

change to using 5kph closure speed for all costings

a01fced

change scooter path in penalty tests after changing closure speed

6ce0359

update changelog

0af73e0

dont use closure speed for calculating duration of traversing a close…

fd1b7ac

…d edge

remove stale comment

e77027f

update costing models missed in last commit

35aab5a

lint fix

cfa43b9

Use a closure factor instead of changing speed for edge costing

a713172

Fix copy-pasta in truckcost

9102cf4

Add configurable closure factor

907fe32

fix doc description of closure_factor

4392cfd

lint fix

9334d7b

mandeepsandhu force-pushed the closure_recosting branch from 44d6c97 to 9334d7b Compare March 30, 2021 16:53

kevinkreiser approved these changes Mar 30, 2021

View reviewed changes

mandeepsandhu merged commit 1b7798a into master Mar 30, 2021

mandeepsandhu deleted the closure_recosting branch April 1, 2021 22:34

dnesbitt61 mentioned this pull request Apr 16, 2021

Performance Drop in Long Bicycle Route #3007

Closed

merkispavel mentioned this pull request Jan 10, 2022

top_speed option: ignore live speed for speed based penalties #3460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Penalize closed edges when routing through them #2964

Penalize closed edges when routing through them #2964

mandeepsandhu commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

kevinkreiser commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021 •

edited

Loading

kevinkreiser commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

danpat commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

danpat commented Mar 24, 2021

kevinkreiser commented Mar 24, 2021

mandeepsandhu commented Mar 25, 2021

mandeepsandhu commented Mar 25, 2021

danpat commented Mar 25, 2021

purew commented Mar 25, 2021

mandeepsandhu commented Mar 25, 2021

kevinkreiser commented Mar 25, 2021 •

edited

Loading

kevinkreiser Mar 26, 2021

mandeepsandhu Mar 26, 2021

kevinkreiser Mar 29, 2021 •

edited

Loading

kevinkreiser Mar 29, 2021 •

edited

Loading

mandeepsandhu Mar 30, 2021

kevinkreiser left a comment

Penalize closed edges when routing through them #2964

Penalize closed edges when routing through them #2964

Conversation

mandeepsandhu commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

kevinkreiser commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021 • edited Loading

kevinkreiser commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

danpat commented Mar 24, 2021

mandeepsandhu commented Mar 24, 2021

danpat commented Mar 24, 2021

kevinkreiser commented Mar 24, 2021

mandeepsandhu commented Mar 25, 2021

mandeepsandhu commented Mar 25, 2021

danpat commented Mar 25, 2021

purew commented Mar 25, 2021

mandeepsandhu commented Mar 25, 2021

kevinkreiser commented Mar 25, 2021 • edited Loading

kevinkreiser Mar 26, 2021

Choose a reason for hiding this comment

mandeepsandhu Mar 26, 2021

Choose a reason for hiding this comment

kevinkreiser Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

kevinkreiser Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

mandeepsandhu Mar 30, 2021

Choose a reason for hiding this comment

kevinkreiser left a comment

Choose a reason for hiding this comment

mandeepsandhu commented Mar 24, 2021 •

edited

Loading

kevinkreiser commented Mar 25, 2021 •

edited

Loading

kevinkreiser Mar 29, 2021 •

edited

Loading

kevinkreiser Mar 29, 2021 •

edited

Loading