control default speed assignment via config #3055

kevinkreiser · 2021-05-05T02:25:28Z

This PR adds initial support for what was proposed in #3021 of note is the change in proposed file format. We'll document that over in the other repo where the code that generates this output can be found.

~~At the moment this PR is still in draft because the unit tests fail. I'm having trouble getting the enhancer to think the gurka map is dense enough to be considered as urban.~~

…ssertions stil

kevinkreiser · 2021-05-06T00:47:11Z

strange that the changelog CI passed... i didnt add a changelog entry yet...

kevinkreiser · 2021-05-06T00:53:23Z

src/mjolnir/graphenhancer.cc

+                   const std::string& country_state_code) {
+
+    // If we have config loaded we'll use that
+    if (!tables.empty() && FromConfig(directededge, density, country_state_code))
+      return;


i highly recommend viewing this diff with whitespace hidden. if you dont do that it will look like i really modified a lot more than i did. what really happened here is i made this anonymous namespace function UpdateSpeed into a member function of SpeedAssigner other than indentation (and so also line wrapping) I didnt modify this function except to call the FromConfig method to first see if the config-driven speed assignment should be used.

so if you run valhalla_build_tiles as normal and don't pass it a speed config file in your regular valhalla json config, the code will hit this if above which will immediately return false and the rest of the unchange code in the UpdateSpeed function will be run as it always was.

kevinkreiser · 2021-05-06T00:54:27Z

src/mjolnir/graphenhancer.cc

+  uint32_t urban_rc_speed[] = {
+      89, // 55 MPH - motorway
+      73, // 45 MPH - trunk
+      57, // 35 MPH - primary
+      49, // 30 MPH - secondary
+      40, // 25 MPH - tertiary
+      35, // 22 MPH - unclassified
+      30, // 20 MPH - residential
+      20, // 13 MPH - service/other
+  };


i did make this change because i thought it was easier to read

kevinkreiser · 2021-05-06T00:54:51Z

src/mjolnir/graphenhancer.cc

+        end_node_code = admin->country_iso();
+        if (!end_node_code.empty())
+          country_state_code += admin->state_iso();


here i got a concatination of country and state codes for lookup in the config

kevinkreiser · 2021-05-06T00:55:23Z

src/mjolnir/graphenhancer.cc

@@ -1360,16 +1538,21 @@ void enhance(const boost::property_tree::ptree& pt,
  // Local Graphreader
  GraphReader reader(hierarchy_properties);

+  // Config driven speed assignment
+  auto speeds_config = pt.get_optional<std::string>("default_speeds");
+  SpeedAssigner speed_assigner(speeds_config);


here we potentially parse the speed config

kevinkreiser · 2021-05-06T00:56:09Z

src/mjolnir/graphenhancer.cc

+        speed_assigner.UpdateSpeed(directededge, density, urban_rc_speed, infer_turn_channels,
+                                   country_state_code);


here we call the anonymous namespace struct to update the speed

kevinkreiser · 2021-05-06T00:56:55Z

src/mjolnir/graphenhancer.cc

-      } else {
-        speed = static_cast<uint32_t>((speed * kRampFactor) + 0.5f);
+struct SpeedAssigner {
+  struct SpeedTable {


this object closely mirrors the json but is easier to use than a rapidjson object so we parse the json into it

kevinkreiser · 2021-05-06T00:57:53Z

src/mjolnir/graphenhancer.cc

+                                            SpeedTable(cs["urban"]),
+                                            SpeedTable(cs["rural"]),


at the moment we have only urban and rural speed tables per each geographic region (country/state combo)

kevinkreiser · 2021-05-06T01:00:06Z

src/mjolnir/graphenhancer.cc

+      tables.clear();
+    } // something else was thrown
+    catch (...) {
+      LOG_WARN("Could not load default speeds from config");
+      tables.clear();


if anything goes wrong when parsing the config its completely disabled

maybe this should be parsed when the tile building starts even before the enhancer stage and fail the whole build?

kevinkreiser · 2021-05-06T01:50:11Z

src/mjolnir/graphenhancer.cc

-    // Reduce speed on rough pavements. TODO - do we want to increase
-    // more on worse surface types?
-    if (directededge.surface() >= Surface::kPavedRough) {
+  bool FromConfig(DirectedEdge& directededge,


this is the main function that either uses a speed from config or aborts and relies on the previous method. anywhere where the method returns false it is signalling that it couldnt assign a speed for this edge. ive commented the sections below to make it pretty clear. if its a kind of edge we know we cant find a speed for either becuase its a special edge that we dont handle or because the geography is not in the config then we abort immediately and rely on the conventional speed setting. once it gets past that point it figures out whether it needs rural or urban speeds. finally it picks which speed to assign.

ill add a javadoc to this function

kevinkreiser · 2021-05-06T03:26:57Z

src/mjolnir/graphenhancer.cc

-                              : static_cast<uint32_t>((speed * kRampFactor) + 0.5f);
-      } else {
-        speed = static_cast<uint32_t>((speed * kRampFactor) + 0.5f);
+struct SpeedAssigner {


anyone have opinions on moving this new class out into a private header?

gknisely · 2021-05-06T12:58:38Z

@kevinkreiser what are the resulting RAD diffs with this PR

kevinkreiser · 2021-05-06T13:07:40Z

@gknisely I wasn't planning on doing RAD for this but I can do a small area if you prefer. Please see my PR comments, especially this one: #3055 (comment) (i've updated the comment a bit for clarity)

The new code is disabled completely and the old code is called as normal so long as you dont pass a speeds config file in your valhalla config json.

…admin

…rocessing application to assign speeds

gknisely

If small RAD testing looked good, then merge. Thanks.

kevinkreiser · 2021-05-08T02:31:33Z

@gknisely ill do a quick RAD and I want to flesh out another unit test to get more coverage of how it handles country/state specific logic

kevinkreiser · 2021-05-10T15:10:21Z

@gknisely i ran RAD on 1k routes i generated from 3 corners of rhode island (providence, newport and westerly) there were 0 diffs. i'm going to add a more extensive unit test and then call it a day on this one.

rzyc

lgtm

kevinkreiser · 2021-05-11T15:05:39Z

src/mjolnir/speed_assigner.h

+
+    // If we have config loaded we'll use that
+    if (!tables.empty() && FromConfig(directededge, density, country_code, state_code)) {
+      directededge.set_speed_type(SpeedType::kClassified);


@rzyc i added this since you last reviewed realizing that it would be set improperly if the speed was a tagged speed

kevinkreiser · 2021-05-11T15:06:09Z

src/mjolnir/speed_assigner.h

+          throw std::runtime_error("Duplicate country/state entry");
+        tables.emplace(std::move(code), std::array<SpeedTable, 3>{
+                                            SpeedTable(cs["rural"]),
+                                            SpeedTable(cs["suburban"]),


we now support a 3rd delinieation of road density

nilsnolde · 2021-05-17T12:47:43Z

@kevinkreiser just had a little time going over this nice addition! Looks really good, thanks for that.

Do I read the code correctly, that, provided a valid JSON exists, the speed enhancer doesn't care whether the edge was tagged or not and updates its speed regardless, i.e. here:

valhalla/src/mjolnir/speed_assigner.h

Lines 257 to 260 in edf42ea

    
           if (!tables.empty() && FromConfig(directededge, density, country_code, state_code)) { 
        
             directededge.set_speed_type(SpeedType::kClassified); 
        
             return true; 
        
           }

If so, why would you prefer to change the tagged speed to a default speed from the JSON? My assumption would be the tagged speed is (hopefully) closer to the ground truth than a country/state-wide default speed.

nilsnolde · 2021-05-17T13:25:11Z

Maybe it could/should be adjustable in the config by a boolean whether one wants to override tagged speeds with the JSON values?

And if tagged speeds won't be overridden by default, how about preparing a default JSON following the "official" OSM recommendations with all values being the same for urban/suburban/rural?

That also reminds me: IMO it should be possible to have missing keys or at least have some keys populated with 0 values which will signal to the algo "don't mind me, use UpdateSpeed to determine my speed". If I'm reading it right, currently it would actually determine 0 edge speed if a JSON value would be 0.

kevinkreiser · 2021-05-17T16:42:10Z

@nilsnolde

My assumption would be the tagged speed is (hopefully) closer to the ground truth than a country/state-wide default speed

I think not actually! The vast vast majority of tagged speeds are maxspeed tags. Speed limit is not a measured speed it is a suggested speed. I would agree that in areas with low traffic volumes most people either drive at or above the speed limit and in these scenarios it is a good proxy for actual travel speed. I suspect that by using a measured speed that in those areas where this is the case we'll probably not much change from what the tagged speed is. Where this will make a difference is in cities or areas with high road congestion (hopefully). I did a manual tuning and tested an area in philly and got much better times out of it. Basically places where the speed limit is aspirational 😄

However you question does resurface a thought that I was having and I think i commented somewhere. We could, instead of putting these values into DirectedEdge::speed_ we could put them into DirectedEdge::constrained_flow_. Normally that is reserved for historical traffic but the whole point of this work is if you dont have real historical traffic it gets you something better than the default speed assignments. Even better why not put all the default speed asignments into constrained_flow_ too and migrate speed_ to just be the speed limit if populated. I think that is why i marked that issue linked above as v4 because it would be a breaking tile spec change.

Regarding the other questions about when and with what to override they are all good points. I'm trying to add this support iteratively meaning I wanted to get something working and merged and come back to do further enhancements and protections which you've listed. I think its ok to not have the perfect solution just yet because the whole thing is opt-in and by default its turned off so no one is being forced into this new code path.

We could add a flag that says dont override tagged speeds as is done for the default speed rules
Ship the official OSM default speeds as a possible default
Add fall back for unknown values to previous speed method or to adjacent geograph or something else...
Handle invalid values in the json

nilsnolde · 2021-05-17T18:17:32Z

The vast vast majority of tagged speeds are maxspeed tags

makes sense:) That does hit close to being able to distinguish maxspeed tags from other speed tags, in which case I'd prefer to keep the tagged speed rather than taking a default. Maybe those tags will be more abundant in the future by automatic data collection from all those rolling 2 ton computers..

I'll help out a little on adding those things, starting with 1. which I can already see clients whining about 😅

kevinkreiser added 4 commits May 4, 2021 16:42

add test and parsing for speeds from config, need to flesh out test a…

f8adf2e

…ssertions stil

add unit tests cases, still failing though

000440f

fix test

eb9d7da

Merge branch 'master' into kk_tweakspeeds

fdb59e4

kevinkreiser marked this pull request as ready for review May 6, 2021 00:46

kevinkreiser requested review from purew, gknisely, genadz and dnesbitt61 May 6, 2021 00:46

kevinkreiser added 4 commits May 5, 2021 20:48

Merge remote-tracking branch 'origin/master' into kk_tweakspeeds

26df449

changelog

971939a

Merge remote-tracking branch 'origin/kk_tweakspeeds' into kk_tweakspeeds

7296103

lint

f2786a1

kevinkreiser commented May 6, 2021

View reviewed changes

more lint

e22640e

kevinkreiser commented May 6, 2021

View reviewed changes

kevinkreiser added 2 commits May 5, 2021 22:03

add javadoc to main function and revert linewrapping changes

16c876a

revert one more line wrapping change

e602dd5

kevinkreiser commented May 6, 2021

View reviewed changes

kevinkreiser requested a review from rzyc May 6, 2021 12:38

kevinkreiser added 5 commits May 6, 2021 13:11

move speed assigner to private header and singleton

ac34e8b

lint

c33969a

fix bug if you build with admins but dont have speed config for that …

aedb14b

…admin

move all speed related constants to speed assigner header. add post p…

2dbe27d

…rocessing application to assign speeds

lint

9f00501

gknisely previously approved these changes May 7, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into kk_tweakspeeds

4a54961

rzyc previously approved these changes May 10, 2021

View reviewed changes

add more test coverage

a4097bf

kevinkreiser dismissed stale reviews from rzyc and gknisely via a4097bf May 10, 2021 20:44

rzyc previously approved these changes May 10, 2021

View reviewed changes

add suburban delineation and more tests

e4c6769

kevinkreiser dismissed rzyc’s stale review via e4c6769 May 11, 2021 15:02

kevinkreiser commented May 11, 2021

View reviewed changes

missed a spot when de-singleton-ifying

ecbed0e

rzyc approved these changes May 11, 2021

View reviewed changes

kevinkreiser merged commit 1e8b53b into master May 11, 2021

nilsnolde mentioned this pull request May 19, 2021

Optionally override tagged speed with the speed assignment JSON #3079

Open

kevinkreiser deleted the kk_tweakspeeds branch January 5, 2022 03:36

kevinkreiser mentioned this pull request Feb 18, 2023

Statistically Improving Default Speed Assignments #3021

Closed

ishibook mentioned this pull request Aug 9, 2023

Seeking Help: Adjusting Default Speed in Valhalla Docker Image (3.4.0) #4247

Closed

chrstnbwnkl mentioned this pull request May 29, 2024

Simple legal default speeds #4739

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

control default speed assignment via config #3055

control default speed assignment via config #3055

kevinkreiser commented May 5, 2021 •

edited

Loading

kevinkreiser commented May 6, 2021

kevinkreiser May 6, 2021 •

edited

Loading

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

kevinkreiser May 6, 2021

gknisely commented May 6, 2021

kevinkreiser commented May 6, 2021 •

edited

Loading

gknisely left a comment

kevinkreiser commented May 8, 2021

kevinkreiser commented May 10, 2021

rzyc left a comment

kevinkreiser May 11, 2021

kevinkreiser May 11, 2021

nilsnolde commented May 17, 2021

nilsnolde commented May 17, 2021

kevinkreiser commented May 17, 2021 •

edited

Loading

nilsnolde commented May 17, 2021

		speed_assigner.UpdateSpeed(directededge, density, urban_rc_speed, infer_turn_channels,
		country_state_code);

control default speed assignment via config #3055

control default speed assignment via config #3055

Conversation

kevinkreiser commented May 5, 2021 • edited Loading

kevinkreiser commented May 6, 2021

kevinkreiser May 6, 2021 • edited Loading

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

gknisely commented May 6, 2021

kevinkreiser commented May 6, 2021 • edited Loading

gknisely left a comment

Choose a reason for hiding this comment

kevinkreiser commented May 8, 2021

kevinkreiser commented May 10, 2021

rzyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde commented May 17, 2021

nilsnolde commented May 17, 2021

kevinkreiser commented May 17, 2021 • edited Loading

nilsnolde commented May 17, 2021

kevinkreiser commented May 5, 2021 •

edited

Loading

kevinkreiser May 6, 2021 •

edited

Loading

kevinkreiser commented May 6, 2021 •

edited

Loading

kevinkreiser commented May 17, 2021 •

edited

Loading