Favor turn channels more #3222

merkispavel · 2021-07-20T12:12:19Z

Issue

At the moment we boost turn channel speeds by some factor

valhalla/src/mjolnir/speed_assigner.h

Line 267 in 5ec6f65

speed = static_cast<uint32_t>((speed * kTurnChannelFactor) + 0.5f);

But we don't do anything with its speeds/costs when there are historic speeds.

PR changes:

reduce kTurnChannel edge cost for both has-historic-speeds and doesn't-have-historic-speeds cases
increase turn channel factor 1.25 -> 1.66. (Note: 1.66 is basically 1 / 0.6)

Screenshots

Before	After

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog
If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

merkispavel · 2021-07-20T12:16:17Z

src/mjolnir/speed_assigner.h

@@ -263,9 +262,7 @@ class SpeedAssigner {
    if (directededge.link()) {
      uint32_t speed = directededge.speed();
      Use use = directededge.use();
-      if (use == Use::kTurnChannel && infer_turn_channels) {
-        speed = static_cast<uint32_t>((speed * kTurnChannelFactor) + 0.5f);


Should be noticed that durations will be increased after removing this line so I'm hesitating if it's the right move..
Alternative safe solution might be

leave speed_assigner code as it is

handle only if (edge.has_historic_speeds) case in the AutoCost::EdgeCost

cons:

Different turn channel factor in 2 places. It would be better to have everything in one place

thats a very good question you raise... here's my opinion:

in general, our speed assignments are always too optimistic. also turn channels are by definition short. i think its maybe its not a big deal to get 20% slower considering these two facts..

also turn channels are by definition short. i think its maybe its not a big deal to get 20% slower considering these two facts

agreed

kevinkreiser · 2021-07-21T02:02:50Z

looks like you need to update the isochrone tests since they change just slightly (i guess they get slightly smaller)

merkispavel · 2021-07-21T08:33:14Z

looks like you need to update the isochrone tests since they change just slightly (i guess they get slightly smaller)

thanks! didn't notice that ci had failed

yeah, it's just 3 points were changed a little bit( +- 1-e4). Fixed 👍

kevinkreiser · 2021-07-21T13:13:43Z

@merkispavel @genadz
one thing i wanted to ask about here was the inverse problem. assuming we are in a right-hand driving country, is it possible that we take a right hand turn channel to make a left turn:

here the route is red. i havent checked but i think this is possible. i would, like uturns, hope that we penalize sharp turns off of turn channels so this only happens if its the only possibility for whatever reason. anyway i was just wondering if you saw anything like that in RAD

merkispavel · 2021-07-21T13:34:49Z

@kevinkreiser
I didn't see such cases in RAD. The reasons might be

there are not such cases in the RAD tests(like in your screenshot). So no one of them tests uturns
Turn channel end road sometimes is one way like in the screenshot below.Even if it's not one way - there's a chance that uturn restriction is already added. Plus, as you said, maybe we already assign increased turn weight for such sharp turns.

I'll double check manually that this PR doesn't add such bugs but I bet I won't find them. Non-oneway end road + no uturn restriction .. the chances are pretty low

)

kevinkreiser · 2021-07-21T14:08:10Z

yep i agree the chances are low, i just wanted to point it out

dnesbitt61 · 2021-07-21T14:12:35Z

the stop_impact is raised when making a sharp turn off of a turn channel - that likely will prevent this.

merkispavel added 2 commits July 20, 2021 06:21

Don't change turnChannel during tile building

9c6df05

Favor turn channels for both predictive/non-predictive speeds

8a5c129

merkispavel commented Jul 20, 2021

View reviewed changes

merkispavel requested review from kevinkreiser and genadz July 20, 2021 12:16

merkispavel added 3 commits July 21, 2021 11:27

Minor fix of the isochrone test

a51d4b6

Merge branch 'master' into tc-factor-v2

748c434

Update CHANGELOG.md

d78aa21

genadz approved these changes Jul 21, 2021

View reviewed changes

merkispavel merged commit 100a2d9 into master Jul 21, 2021

merkispavel mentioned this pull request Jul 23, 2021

Revert default speed boost for turn channels #3232

Merged

5 tasks

nilsnolde deleted the tc-factor-v2 branch February 24, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Favor turn channels more #3222

Favor turn channels more #3222

merkispavel commented Jul 20, 2021

merkispavel Jul 20, 2021 •

edited

Loading

kevinkreiser Jul 21, 2021

merkispavel Jul 21, 2021

kevinkreiser commented Jul 21, 2021

merkispavel commented Jul 21, 2021

kevinkreiser commented Jul 21, 2021

merkispavel commented Jul 21, 2021 •

edited

Loading

kevinkreiser commented Jul 21, 2021

dnesbitt61 commented Jul 21, 2021

Favor turn channels more #3222

Favor turn channels more #3222

Conversation

merkispavel commented Jul 20, 2021

Issue

Screenshots

Tasklist

Requirements / Relations

merkispavel Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

kevinkreiser Jul 21, 2021

Choose a reason for hiding this comment

merkispavel Jul 21, 2021

Choose a reason for hiding this comment

kevinkreiser commented Jul 21, 2021

merkispavel commented Jul 21, 2021

kevinkreiser commented Jul 21, 2021

merkispavel commented Jul 21, 2021 • edited Loading

kevinkreiser commented Jul 21, 2021

dnesbitt61 commented Jul 21, 2021

merkispavel Jul 20, 2021 •

edited

Loading

merkispavel commented Jul 21, 2021 •

edited

Loading