Add logic to penalize certain roads in the car profile #3636

TheMarex · 2017-02-01T10:34:48Z

Continuation of #3276 against the right branch.

Issue

In the past we have used penalties on the speed to make routing over certain types of roads less appealing to the routing engine. However this screws with the ETAs when you actually need to go over those penalized roads.

This PR changes this speed penalties to weight penalties which was unlocked by fixing #77.

This means certain tests that test for applying the penalties in the car profile will fail now. They need to be adapted to test if the speed is unchanged while the rate/weight is adjusted for the penalty.

Tasklist

port hov handler into penalty handler to heavily penalize HOV tagged ways
use min applicable penalty rather than a multiplied aggregate penalty value
Add support for new cucumber test headers: rate and weight
Update tests to test for more important values, e.g. weight rather than speed
add regression / cucumber cases (see docs/testing.md)
review
adjust for comments

emiltin · 2017-02-03T13:19:57Z

profiles/lib/handlers.lua

+    sideroad_penalty = side_road_multipler;
+  end
+
+  local penalty = service_penalty * width_penalty * alternating_penalty * sideroad_penalty


i haven't worked with weights yet, but for speed reductions my experience is that multiplying penalites doesn't give good results. it's better to use mininimum. the problem is at a modest reduction for all parameters results in a a very low speed, effectually causing the way to be unroutable.

👍 Updated to use the minimum of all set penalties

daniel-j-h · 2017-02-09T17:16:57Z

unit_tests/library/tile.cpp

@@ -32,7 +32,7 @@ BOOST_AUTO_TEST_CASE(test_tile)
    const auto rc = osrm.Tile(params, result);
    BOOST_CHECK(rc == Status::Ok);

-    BOOST_CHECK_EQUAL(result.size(), 114033);
+    BOOST_CHECK_EQUAL(result.size(), 113824);


Why does this change?

I think it's because the duration values encoded in the vector tiles changed. It uses a delta encoding so the size is heavily data dependent.

daniel-j-h · 2017-02-09T17:18:43Z

features/step_definitions/routability.js

@@ -31,6 +59,8 @@ module.exports = function () {
                                usingShortcut = row[direction];
                            }

+                            // TODO split out accessible/not accessible value from forw/backw headers
+                            // rename forw/backw to forw/backw_speed


Can you ticket this

daniel-j-h · 2017-02-09T17:19:22Z

features/step_definitions/routability.js

+                                if (result[direction].rate) {
+                                    outputRow[rate] = !isNaN(result[direction].rate) ?
+                                        result[direction].rate.toString() :
+                                        result[direction].rate.toString() || '';


Looks strange, is line 39 required?

daniel-j-h · 2017-02-09T17:21:28Z

profiles/lib/handlers.lua

+    sideroad_penalty = side_road_multiplier;
+  end
+
+  local penalty = math.min(service_penalty, width_penalty, alternating_penalty, sideroad_penalty, hov_penalty)


Not sure if this is semantically correct - e.g. a narrow alternating oneway should have a higher penalty than a narrow street, right?

One of Emil's concerns was that this usually leads to roads becoming unroutable instead of penalized.

it should be based on considering the nature of the inputs.
regarding speed: if a way is narrow and has a bad surface, it's probably not going to be slower than if it's just narrow. on the other hand, alternating directions means you're going to spend time standing still, so it might make sense to multiple that factor, e.g speed = speed*min(width,surface)*alternating

danpat · 2017-02-10T12:59:19Z

features/car/service.feature

-            | service  | parking           | 5 km/h +-1 | 5 km/h +-1  |
+            | highway  | service           | forw        | backw        | forw_rate  |
+            | service  | alley             | 15 km/h +-1 | 15 km/h +-1  | 2          |
+            | service  | emergency_access  |             |              |            |


Now that we have the ability to penalize access-limited ways, should emergency_access just become like all the others (i.e. allow routing if a pin is dropped) ?

On second thought, I guess not - cut-off roads on freeways that are emergency-only should never be routed over, and because they're short, even heavily penalizing them is not sufficient to prevent access. We would need to implement turn-based penalties for that to work.

So, nevermind, but I've written all this stuff now, so I'm going to post it anyway for perpetuity.

TheMarex added the Work In Progress label Feb 1, 2017

karenzshea force-pushed the feature/avoid-service-roads branch from 4fd41a7 to 9cb880b Compare February 3, 2017 09:37

karenzshea changed the title ~~Add logic to penaltize certain roads in the car profile~~ Add logic to penalize certain roads in the car profile Feb 3, 2017

emiltin reviewed Feb 3, 2017

View reviewed changes

karenzshea force-pushed the feature/avoid-service-roads branch 3 times, most recently from 5ac07bc to 9600ae8 Compare February 6, 2017 11:59

TheMarex added this to the 5.6.0 milestone Feb 7, 2017

karenzshea force-pushed the feature/avoid-service-roads branch 4 times, most recently from f29a173 to 95f494c Compare February 9, 2017 14:13

karenzshea added Review and removed Work In Progress labels Feb 9, 2017

TheMarex force-pushed the feature/avoid-service-roads branch from 95f494c to a039549 Compare February 9, 2017 15:45

karenzshea force-pushed the feature/avoid-service-roads branch from a039549 to 95f494c Compare February 9, 2017 16:14

karenzshea added 6 commits February 9, 2017 16:15

fix merge conflicts

5a37212

remove calls to handlers now in handle_penalties

93717c6

hov_penalty, use min penalty rather than aggregate

efd5282

provide rate and weight headers for checking in routability tests

2bf4d65

convert speed to meters per second when used to set rate

47c0b1a

remove units from rate columns in routability testing

85d330d

TheMarex force-pushed the feature/avoid-service-roads branch from 95f494c to 85d330d Compare February 9, 2017 16:15

TheMarex requested a review from danpat February 9, 2017 16:23

karenzshea requested a review from daniel-j-h February 9, 2017 16:36

daniel-j-h reviewed Feb 9, 2017

View reviewed changes

danpat reviewed Feb 10, 2017

View reviewed changes

TheMarex added 2 commits February 10, 2017 13:25

Fix HOV lane support with penalties

af33891

Make rate support code a little bit more obvious

5b783ba

TheMarex force-pushed the feature/avoid-service-roads branch from 4e7bccc to 5b783ba Compare February 10, 2017 13:26

TheMarex added Ready To Merge and removed Review labels Feb 10, 2017

TheMarex merged commit d28713b into master Feb 10, 2017

TheMarex deleted the feature/avoid-service-roads branch February 10, 2017 14:27

SiarheiFedartsou mentioned this pull request Nov 3, 2022

Difference between Routability and Duration weight setting #6440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logic to penalize certain roads in the car profile #3636

Add logic to penalize certain roads in the car profile #3636

TheMarex commented Feb 1, 2017 •

edited

Loading

emiltin Feb 3, 2017

karenzshea Feb 6, 2017

daniel-j-h Feb 9, 2017

TheMarex Feb 10, 2017

daniel-j-h Feb 9, 2017

TheMarex Feb 10, 2017

daniel-j-h Feb 9, 2017

daniel-j-h Feb 9, 2017

TheMarex Feb 10, 2017

emiltin Feb 10, 2017

danpat Feb 10, 2017

Add logic to penalize certain roads in the car profile #3636

Add logic to penalize certain roads in the car profile #3636

Conversation

TheMarex commented Feb 1, 2017 • edited Loading

Issue

Tasklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMarex commented Feb 1, 2017 •

edited

Loading