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

merkispavel · 2021-12-15T12:34:04Z

Description

top_speed option does 2 things:

limits edge speed with the given value
penalizes edges with higher speed

This PR touches penalty logic of the top_speed. In the context of speed based penalties, these changes forces edge speed to be taken from "static" speed layers rather than dynamic ones(current, predictive)

Why

Live traffic speed is dynamic and can change quickly. For example, if you only want to use roads below 40kph, a high speed motorway may have big congestion with speeds at 20kph but it is possible that by the time the vehicle reaches the motorway, the traffic clears up and live speed returns back to normal 100kph
In my view, the same logic is applied to predicted speeds as well

Questions to reviewers

Does this change make sense to you?
Are changes consistent with original design of top_speed option(if there was any idea behind it)?

Next steps

If everybody is ok with this changes - I'll apply these changes to other places(other costings) and probably add some tests

nilsnolde · 2021-12-15T16:47:01Z

hm, maybe I'm not thinking right about this, also not very familiar with time-dependent astar, but anyways: the changes make total sense to me in case of live traffic. we simply don't know what the speed will be by the time the car actually gets there. but for predicted speed we do know the right speed at the right time during traversal no? to me it seems that's cool to use for speed-based penalization, happy to hear your thoughts:)

genadz · 2021-12-16T08:46:33Z

src/sif/autocost.cc

+    average_historic_speed = tile->GetSpeed(edge, flow_mask_ & kStaticFlowMask);
+  }
+  float speed_penalty =
+      (average_historic_speed > top_speed_) ? (edge_speed - top_speed_) * 0.05f : 0.0f;


Suggested change

(average_historic_speed > top_speed_) ? (edge_speed - top_speed_) * 0.05f : 0.0f;

(average_historic_speed > top_speed_) ? (average_historic_speed - top_speed_) * 0.05f : 0.0f;

?
otherwise, if edge_speed < top_speed_ we can get negative speed penalty

ah, right. Thanks @genadz

genadz · 2021-12-16T09:05:38Z

hm, maybe I'm not thinking right about this, also not very familiar with time-dependent astar, but anyways: the changes make total sense to me in case of live traffic. we simply don't know what the speed will be by the time the car actually gets there. but for predicted speed we do know the right speed at the right time during traversal no? to me it seems that's cool to use for speed-based penalization, happy to hear your thoughts:)

valid note. it depends what is the goal of top_speed parameter. I see 2 situations:

we want to penalize roads where actual speed will be higher than top_speed - in this case predicted speeds should be taken because we approximately know what speed will be on the edge;
we want to penalize roads where actual speed may be higher than top_speed - here we should take some static data.

nilsnolde · 2021-12-16T13:57:12Z

thanks. so basically option 1 is saying "we trust the speed data completely so please give me the (however accurate) speed at that time and edge".. for option 2 we'd assume (rightfully I guess) that the speeds are inaccurate bcs time will be off due to unexpected live events along the expansion etc and rather take the "safe" approach of the static average speed, constrained or free.

ha, that's more of a philosophical question than I expected 😅 since in the case where this applies we're not using the original edge_speed but the top_speed_ we really use it only to penalize, not for the base cost. still, I think I'd prefer actually option 1, because in cases where the edge_speed is lower than top_speed_ we also pretend to trust the dynamic speed values enough to take it as base cost. so to me that'd be more consistent no? but then raises actually the same conclusion for realtime speeds, ie the whole PR. anyways, just my thoughts, hoping for people with more experience to chime in. it's also not such a huge decision, so sorry for maybe overthinking this..

kevinkreiser · 2021-12-16T16:39:20Z

the way i see it was more that it is a matter of the legal speed not the measured speed. if my top speed is 65mph and the speed limit is 55mph, but crazy people are driving 85mph I dont think i should be penalized and suggested to stay off that road. On the other hand if I'm conservative maybe I do want to avoid the crazy people so they don't run me over (an argument I often hear from my pappy).

nilsnolde · 2021-12-16T16:51:30Z

but doesn't the same thought also apply to using "measured" (live/historical) speed in general? using it as base cost has the same implications no?

kevinkreiser · 2021-12-16T16:59:06Z

i guess my point is that setting top speed should cap the travel speed to that, so measured speed will be capped when used as a base cost. my understanding was that when a user says my top speed is X, they are telling us I cannot drive faster than X, even if the measure speed is X + 20, i will be driving X.

nilsnolde · 2021-12-16T17:11:49Z

aaah now I got it, yes.. makes sense, sorry for the noise!

merkispavel · 2021-12-16T18:27:39Z

the way i see it was more that it is a matter of the legal speed not the measured speed
agreed, this is how I think about it as well

if my top speed is 65mph and the speed limit is 55mph, but crazy people are driving 85mph I dont think i should be penalized and suggested to stay off that road.

makes total sense. Any historic speed source(predicted even predicted one with N mins buckets) should smooth such extreme values(like 85mph on a road with 55mph speed limit)

merkispavel · 2021-12-16T18:32:03Z

Looking at your thoughts, I would suggest to move one speed layer at a time. Let's disable live speeds(i.e current) source for top_speed feature and see how it goes
I'll update the PR soon

merkispavel · 2021-12-21T11:15:43Z

I update the PR, folks. It's ready for review

kevinkreiser · 2021-12-21T15:46:30Z

src/sif/autocost.cc

+  // better to use layers with smoothed/constant speeds
+  if (top_speed_ != kMaxAssumedSpeed && (flow_sources & kCurrentFlowMask)) {
+    average_edge_speed =
+        tile->GetSpeed(edge, flow_mask_ & kNotCurrentFlowMask, time_info.second_of_week);


i think this should be:

Suggested change

tile->GetSpeed(edge, flow_mask_ & kNotCurrentFlowMask, time_info.second_of_week);

tile->GetSpeed(edge, flow_mask_ & (~kCurrentFlowMask), time_info.second_of_week);

its less confusing that way imho

Sure, I'll drop this alias

kevinkreiser · 2021-12-21T15:46:57Z

valhalla/baldr/graphconstants.h

@@ -629,6 +629,7 @@ constexpr uint8_t kFreeFlowMask = 1;
 constexpr uint8_t kConstrainedFlowMask = 2;
 constexpr uint8_t kPredictedFlowMask = 4;
 constexpr uint8_t kCurrentFlowMask = 8;
+constexpr uint8_t kNotCurrentFlowMask = kFreeFlowMask | kConstrainedFlowMask | kPredictedFlowMask;


i dont th ink we need this we can just use bitwise operators directly right?

I don't mind. I didn't measure but I hope it won't impact performance

bitwise negation of a constant? if the compiler doesnt optimize that then ill be a monkeys uncle!

Oh, damn, indeed.. I forgot that they are constants(constexpr-s). The complier definitely handles such things 👍 Thanks for the hint, @kevinkreiser

kevinkreiser

this looks fine to me, i suggested a small change to use negation to flip the current flow bit rather than having another constant.

The compiler is expected to handle any constant operations so there's no need to bring new variables

kevinkreiser · 2021-12-21T18:46:32Z

Shouldn't we do other motorized costings like truck etc?

merkispavel · 2021-12-23T22:06:17Z

Shouldn't we do other motorized costings like truck etc?

My bad. Originally I grepped only autocost.cc file for top_speed_ usages.
Now all costings(truck, motor*, etc) have these change. Nice catch @kevinkreiser 🙏

mandeepsandhu · 2021-12-23T23:32:38Z

Some of the closure penalty tests are failing. I remember we penalized closed roads by costing it as a very low-speed road. Looks like that penalty is no longer applied?

merkispavel · 2022-01-10T12:31:52Z

test/gurka/test_closure_penalty.cc

@@ -172,9 +172,6 @@ TEST_P(ClosurePenalty, AvoidClosure) {
            .str();

    std::vector<std::string> expected_path = {"QT", "TU", "RU", "OR", "LO", "IL", "FI", "CF"};
-    if (costing == "motor_scooter") {


@mandeepsandhu I've seen a comment about this change in your PR #2964 (comment)
I guess this is insignificant(as you noted)

merkispavel · 2022-01-10T22:28:10Z

Tests are finally fixed. All tests pass and the PR ready for the final review.
FYI I added on more commit to extract speed penalty calculation and avoid its duplication across costings. Let me know if this is redundant: it's a standalone commit so it's easy to revert

mandeepsandhu

lgtm

merkispavel · 2022-01-11T09:44:04Z

Going to merge it, folks. Feel free to leave comments/review if there's something that needs to be fixed/reverted/etc

nilsnolde · 2022-01-11T09:48:29Z

no worries, was waiting for tests passing before approving. good with me, thanks!

kevinkreiser · 2022-01-11T14:14:42Z

yep you covered my comments so i was good thanks

* Not use live/predicted speeds for speed penalties * Drop only live speeds from top_speed penalties * Apply top speed changes for TaxiCost * remove unnecessary dynamicflowmask anymore * Test that live speed is disabled to top_speed option * Update CHANGELOG.md * Drop newly added flow constant, use bitwise negation inplace The compiler is expected to handle any constant operations so there's no need to bring new variables * Apply top_speed changes to other costings * Fix closure test: use common expected route for motorscooter * Refactor: extract speed based penalty calculation

* sequester outputs of location correlation in pbf * missed a spot * refactor costing protos * lint and docs * fix changelog * top_speed option: ignore live speed for speed based penalties (#3460) * Not use live/predicted speeds for speed penalties * Drop only live speeds from top_speed penalties * Apply top speed changes for TaxiCost * remove unnecessary dynamicflowmask anymore * Test that live speed is disabled to top_speed option * Update CHANGELOG.md * Drop newly added flow constant, use bitwise negation inplace The compiler is expected to handle any constant operations so there's no need to bring new variables * Apply top_speed changes to other costings * Fix closure test: use common expected route for motorscooter * Refactor: extract speed based penalty calculation * allow the caller to ask for only certain parts of the top level protobuf api * Include roads under construction into the graph (#3455) * Use roads under construction * updated lua code to include roads under construction; * added a new config option to include or exclude roads under construction from valhalla graph; * exclude roads under construction from routing; * exclude roads under construction from shortcuts building. * Added tests for roads under construction * Update changelog * Update taginfo.json * Add 'hov' costing to test constructions * Put 'construction' use above the transit uses * Turn off access for roads under construction * Remove needless 's' from the option 'include_constructions' * Turn off access for construction in lua; - exclude construction from density calculation; - do not use construction as an intersecting edge; * Exclude roads under construction from stopimpact calculation * Fix isochrone test * Update changelog * allow the caller to ask for only certain parts of the top level protobuf api * if you want pbf and you are sending json as input we need to parse the format string * changelog merge again * changelog merge again * have the main parsing function do the request clearing in one place rather than duplicating * when not specified default to minimal pbf return for pbf formats. update tests to prove it * kick ci * fix mac build * changelog formatting * changelog formatting Co-authored-by: Pavel Merkis <pavel.merkis@mapbox.com> Co-authored-by: Henadzi Klimuk <genadz.klimuk@mapbox.com>

merkispavel requested review from nilsnolde, kevinkreiser and genadz December 15, 2021 12:34

Not use live/predicted speeds for speed penalties

964c65c

merkispavel force-pushed the drop-current-flow-avoidance branch from 2174d7c to 964c65c Compare December 15, 2021 12:35

Merge branch 'master' into drop-current-flow-avoidance

6debd0e

genadz reviewed Dec 16, 2021

View reviewed changes

merkispavel added 5 commits December 17, 2021 02:28

Drop only live speeds from top_speed penalties

195ad7d

Apply top speed changes for TaxiCost

0cc224f

remove unnecessary dynamicflowmask anymore

19e4337

Test that live speed is disabled to top_speed option

9b0222f

Merge branch 'master' into drop-current-flow-avoidance

643bf91

merkispavel changed the title ~~top_speed option: don't use dynamic speed layers for speed based penalties~~ top_speed option: ignore live speed for speed based penalties Dec 20, 2021

Update CHANGELOG.md

ef8f1cb

merkispavel requested a review from genadz December 21, 2021 11:15

kevinkreiser reviewed Dec 21, 2021

View reviewed changes

kevinkreiser previously approved these changes Dec 21, 2021

View reviewed changes

Drop newly added flow constant, use bitwise negation inplace

e7f2752

The compiler is expected to handle any constant operations so there's no need to bring new variables

merkispavel dismissed kevinkreiser’s stale review via e7f2752 December 21, 2021 15:57

Merge branch 'master' into drop-current-flow-avoidance

d9d41ca

Apply top_speed changes to other costings

bc733a2

merkispavel requested a review from kevinkreiser December 23, 2021 22:06

merkispavel added 2 commits January 10, 2022 15:28

Fix closure test: use common expected route for motorscooter

4a7781c

Refactor: extract speed based penalty calculation

e5bc953

merkispavel commented Jan 10, 2022

View reviewed changes

Merge branch 'master' into drop-current-flow-avoidance

da6d858

mandeepsandhu approved these changes Jan 11, 2022

View reviewed changes

merkispavel merged commit b5ce7c4 into master Jan 11, 2022

merkispavel deleted the drop-current-flow-avoidance branch January 11, 2022 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

merkispavel commented Dec 15, 2021 •

edited

Loading

nilsnolde commented Dec 15, 2021 •

edited

Loading

genadz Dec 16, 2021

merkispavel Dec 16, 2021

genadz commented Dec 16, 2021

nilsnolde commented Dec 16, 2021 •

edited

Loading

kevinkreiser commented Dec 16, 2021

nilsnolde commented Dec 16, 2021 •

edited

Loading

kevinkreiser commented Dec 16, 2021

nilsnolde commented Dec 16, 2021

merkispavel commented Dec 16, 2021

merkispavel commented Dec 16, 2021

merkispavel commented Dec 21, 2021

kevinkreiser Dec 21, 2021 •

edited

Loading

merkispavel Dec 21, 2021

kevinkreiser Dec 21, 2021

merkispavel Dec 21, 2021

kevinkreiser Dec 21, 2021

merkispavel Dec 21, 2021 •

edited

Loading

kevinkreiser left a comment

kevinkreiser commented Dec 21, 2021

merkispavel commented Dec 23, 2021

mandeepsandhu commented Dec 23, 2021

merkispavel Jan 10, 2022

merkispavel commented Jan 10, 2022

mandeepsandhu left a comment

merkispavel commented Jan 11, 2022

nilsnolde commented Jan 11, 2022

kevinkreiser commented Jan 11, 2022

	(average_historic_speed > top_speed_) ? (edge_speed - top_speed_) * 0.05f : 0.0f;
	(average_historic_speed > top_speed_) ? (average_historic_speed - top_speed_) * 0.05f : 0.0f;

	tile->GetSpeed(edge, flow_mask_ & kNotCurrentFlowMask, time_info.second_of_week);
	tile->GetSpeed(edge, flow_mask_ & (~kCurrentFlowMask), time_info.second_of_week);

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

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

Conversation

merkispavel commented Dec 15, 2021 • edited Loading

Description

Why

Questions to reviewers

Next steps

nilsnolde commented Dec 15, 2021 • edited Loading

genadz Dec 16, 2021

Choose a reason for hiding this comment

merkispavel Dec 16, 2021

Choose a reason for hiding this comment

genadz commented Dec 16, 2021

nilsnolde commented Dec 16, 2021 • edited Loading

kevinkreiser commented Dec 16, 2021

nilsnolde commented Dec 16, 2021 • edited Loading

kevinkreiser commented Dec 16, 2021

nilsnolde commented Dec 16, 2021

merkispavel commented Dec 16, 2021

merkispavel commented Dec 16, 2021

merkispavel commented Dec 21, 2021

kevinkreiser Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

merkispavel Dec 21, 2021

Choose a reason for hiding this comment

kevinkreiser Dec 21, 2021

Choose a reason for hiding this comment

merkispavel Dec 21, 2021

Choose a reason for hiding this comment

kevinkreiser Dec 21, 2021

Choose a reason for hiding this comment

merkispavel Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

kevinkreiser left a comment

Choose a reason for hiding this comment

kevinkreiser commented Dec 21, 2021

merkispavel commented Dec 23, 2021

mandeepsandhu commented Dec 23, 2021

merkispavel Jan 10, 2022

Choose a reason for hiding this comment

merkispavel commented Jan 10, 2022

mandeepsandhu left a comment

Choose a reason for hiding this comment

merkispavel commented Jan 11, 2022

nilsnolde commented Jan 11, 2022

kevinkreiser commented Jan 11, 2022

merkispavel commented Dec 15, 2021 •

edited

Loading

nilsnolde commented Dec 15, 2021 •

edited

Loading

nilsnolde commented Dec 16, 2021 •

edited

Loading

nilsnolde commented Dec 16, 2021 •

edited

Loading

kevinkreiser Dec 21, 2021 •

edited

Loading

merkispavel Dec 21, 2021 •

edited

Loading