added use_lit costing option for pedestrian #3957

chrstnbwnkl · 2023-02-09T08:43:27Z

Issue

Adds a use_lit costing option to pedestrian costing, see discussion in #3848.

The use_lit value is converted into an avoid_unlit factor using the following formula:

 unlit_factor_ = use_lit < 0.5f ? 1 + (kMinLitFactor + 2.f * use_lit) : ((kMinLitFactor - 5) + 12.f * use_lit);

Which maps the range [0, 1] to the factor range [1, 8], so that use_lit=0 has no effect on the route, 0 < use_lit < 0.5 has some effect on the route and 0.5 ≤ use_lit ≤ 1 has a comparatively stronger effect on the route.

use_lit is defined in dynamiccost so it could be used as well by e.g. bicycle costing, the exact implementation for which would need to be discussed (as an avid cyclist myself I could see a necessity for this, as the degree to which a cyclist can be seen by other road users greatly determines safety at night).

Tasklist

Add tests
[x ] 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?

nilsnolde

Thanks, looks good to me! Just a tiny nit.

Next PR(s) can deal with

adding additional penalties if the edge is a tunnel and unlit
adding use_lit to bicycle costing, what do you think @dnesbitt61 ?

I personally don't think RAD testing is very necessary, though it'll be called on most edges which are ped accessible. Also its impact is fairly well imaginable IMO.

nilsnolde · 2023-02-09T08:51:54Z

src/sif/dynamiccost.cc

@@ -354,6 +358,11 @@ void DynamicCost::set_use_living_streets(float use_living_streets) {
             2.f * (1.f - use_living_streets) * (1.f - kMinLivingStreetFactor));
 }

+void DynamicCost::set_use_lit(float use_lit) {
+  unlit_factor_ =
+      use_lit < 0.5f ? 1 + (kMinLitFactor + 2.f * use_lit) : ((kMinLitFactor - 5) + 12.f * use_lit);


nit: use float everywhere to avoid implicit conversions

nilsnolde · 2023-02-09T09:06:15Z

proto/options.proto

    repeated AvoidEdge exclude_edges = 78;
    oneof has_elevator_penalty {
      float elevator_penalty = 79;
    }
    uint32 fixed_speed = 80;
    uint32 axle_count = 81;
+    float use_lit = 82;


looks like we have to do it a oneof here to satisfy the JSON_PBF_RANGED_DEFAULT macro @chrstnbwnkl , even though it wouldn't actually be necessary.

ha, funny, reverting actually pulls up my initial comment again..

proto/options.proto

kevinkreiser · 2023-02-09T13:36:59Z

proto/options.proto

+    oneof has_use_lit {
+      float use_lit = 82;
+    }


if the default is 0.f then you dont need to use oneof because both not being set and the default are one and the same value. i know that all the other ones, even the ones that default to 0 are using oneof but thats only because i didnt get a chance to go back and do these, you could be the guy to start the wave :D

went the way of a new macro and no oneof

kevinkreiser · 2023-02-09T13:51:56Z

src/sif/pedestriancost.cc

+  if (!edge->lit()) {
+    factor *= unlit_factor_;
+  }


this may be a preoptimization however, it is possible to do this without a branch (if), you can cast the bool to float and multiply with it instead which may help performance somewhat, it certainly wont be slower than the branch statement.

Suggested change

if (!edge->lit()) {

factor *= unlit_factor_;

}

factor *= (!edge->lit()) * unlit_factor_;

this is bullcrap sorry, didnt think about hard enough, when it is lit we need the factor to still be 1, so we need some slightly more complicated math to avoid the if i'll respond later, you can ignore this for now haha

ok this is my actual suggestion to remove the branch:

factor *= edge->lit() + (!edge->lit() * unlit_factor_);

(maybe just forget about it haha)

@chrstnbwnkl actually implemented your suggestion:)

nilsnolde · 2023-02-09T17:33:33Z

The completely unrelated astar_bss test is failing.. Since I can't figure out what he's complaining about in-code and the VS Code debugger is a piece of **** (and I don't want to setup CLion right now or go back and forth with gdb's cheat sheet), this will have to wait until tmrw or so.

kevinkreiser · 2023-02-10T14:54:20Z

is it possible we changed costing in the default case? that would make the bss path different for the pedestrian part potentially. i would have expected the code we added would simply be factor *= 1. but perhaps we are wrong about that

nilsnolde · 2023-02-10T15:52:14Z

is it possible we changed costing in the default case?

That's what I figured too, but I just can't see it happening in the code changes.. Still can't really debug, but hopefully on the weekend I'll have more time.

nilsnolde

This is done now. I also renamed the astar_bss.cc test file to astar_bikeshare.cc as that caused problems with VS Code's gdb implementation.

nilsnolde · 2023-02-14T18:20:05Z

src/sif/dynamiccost.cc

@@ -354,6 +358,11 @@ void DynamicCost::set_use_living_streets(float use_living_streets) {
             2.f * (1.f - use_living_streets) * (1.f - kMinLivingStreetFactor));
 }

+void DynamicCost::set_use_lit(float use_lit) {
+  unlit_factor_ =
+      use_lit < 0.5f ? kMinLitFactor + 2.f * use_lit : ((kMinLitFactor - 5.f) + 12.f * use_lit);


but I just can't see it happening in the code changes.

OK, I'm just blind (again).. There was a tiny bug there @chrstnbwnkl which caused the default to be a unlit_factor_ of 2 (as it must've been, not sure why I didn't see it..).

gknisely · 2023-02-15T14:34:11Z

src/sif/dynamiccost.cc

@@ -92,6 +95,7 @@ constexpr float kDefaultUseFerry = 0.5f;         // Default preference of using
 constexpr float kDefaultUseRailFerry = 0.4f;     // Default preference of using a rail ferry 0-1
 constexpr float kDefaultUseTracks = 0.5f;        // Default preference of using tracks 0-1
 constexpr float kDefaultUseLivingStreets = 0.1f; // Default preference of using living streets 0-1
+constexpr float kDefaultUseLit = 0.f;            // Default preference of using lit ways 0-1


@chrstnbwnkl why a default of 0.f vs 0.5f? Meaning no preference by default.

The concept with lit is to avoid unlit edges and not do any modulation of lit edges. Then 0 has no influence at all, so it's fully opt-in for clients.

gknisely · 2023-02-15T14:37:04Z

src/sif/pedestriancost.cc

@@ -89,6 +89,8 @@ const std::string kDefaultPedestrianType = "foot";
 // hills) to 1 (take hills when they offer a more direct, less time, path).
 constexpr float kDefaultUseHills = 0.5f;

+constexpr float kDefaultUseLit = 0.f;


@chrstnbwnkl same comment as above. Why not 0.5f for the default.

@chrstnbwnkl The reason I am asking about this is that I am wondering what the differences are between master and this branch when running pedestrian routes from here https://github.com/valhalla/valhalla/tree/master/test_requests I am wondering if there could be a negative impact. May need to add new tests with areas that have lit tag prevalent and areas where it is not.

If you're not changing the use_lit request option there shouldn't be any differences at all compared to master. Only when you start with use_lit > 0 in costing options, there'll be differences. We thought we could avoid doing a RAD testing tbh 😇 since the impact is very well imaginable IMO, see the graph in @chrstnbwnkl 's OP. Do let us know if you're somewhat concerned, maybe we can talk about it tmrw in the 8:30 review session if you have the time @gknisely ?

Also there's tests for BSS handling which were failing when we had the tiny bug where the default use_lit wasn't zero, but 0.1 or so. That's requesting against a tileset in Paris and it ends up taking different paths, even with low use_lit values.

chrstnbwnkl added 2 commits February 9, 2023 09:25

added use_lit costing option for pedestrian

16789d4

changelog

38eb6a5

nilsnolde previously approved these changes Feb 9, 2023

View reviewed changes

nilsnolde reviewed Feb 9, 2023

View reviewed changes

comments

16f7431

chrstnbwnkl dismissed nilsnolde’s stale review via 16f7431 February 9, 2023 10:26

nilsnolde reviewed Feb 9, 2023

View reviewed changes

proto/options.proto Outdated Show resolved Hide resolved

keep order in options.proto

4a99eec

kevinkreiser reviewed Feb 9, 2023

View reviewed changes

remove oneof again and introduce V2 of macro

eadc4a8

kevinkreiser previously approved these changes Feb 10, 2023

View reviewed changes

avoid branching in pedestrian edge cost

461a422

chrstnbwnkl dismissed kevinkreiser’s stale review via 461a422 February 10, 2023 15:57

nilsnolde added 3 commits February 14, 2023 18:59

Merge remote-tracking branch 'upstream/master' into cb-lit-costing

3b423d4

rename test file; fix bug

4647b52

rename test file; fix bug

bab0b3c

nilsnolde previously approved these changes Feb 14, 2023

View reviewed changes

nilsnolde requested a review from kevinkreiser February 14, 2023 18:21

nilsnolde dismissed their stale review via 24282d6 February 14, 2023 18:22

changelog

24282d6

nilsnolde self-requested a review February 14, 2023 18:26

nilsnolde approved these changes Feb 14, 2023

View reviewed changes

gknisely reviewed Feb 15, 2023

View reviewed changes

kevinkreiser approved these changes Feb 16, 2023

View reviewed changes

nilsnolde merged commit 7854707 into valhalla:master Feb 16, 2023

nilsnolde deleted the cb-lit-costing branch February 16, 2023 12:40

nilsnolde mentioned this pull request May 17, 2023

Bicycle network preference #3674

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added use_lit costing option for pedestrian #3957

added use_lit costing option for pedestrian #3957

chrstnbwnkl commented Feb 9, 2023

nilsnolde left a comment

nilsnolde Feb 9, 2023

nilsnolde Feb 9, 2023

nilsnolde Feb 9, 2023 •

edited

Loading

kevinkreiser Feb 9, 2023

nilsnolde Feb 9, 2023

kevinkreiser Feb 9, 2023

kevinkreiser Feb 9, 2023

kevinkreiser Feb 10, 2023

kevinkreiser Feb 10, 2023

nilsnolde Feb 14, 2023

nilsnolde commented Feb 9, 2023 •

edited

Loading

kevinkreiser commented Feb 10, 2023

nilsnolde commented Feb 10, 2023

nilsnolde left a comment

nilsnolde Feb 14, 2023

gknisely Feb 15, 2023

nilsnolde Feb 15, 2023

gknisely Feb 15, 2023

gknisely Feb 15, 2023

nilsnolde Feb 15, 2023

nilsnolde Feb 15, 2023

added use_lit costing option for pedestrian #3957

added use_lit costing option for pedestrian #3957

Conversation

chrstnbwnkl commented Feb 9, 2023

Issue

Tasklist

Requirements / Relations

nilsnolde 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 Feb 9, 2023 • 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

nilsnolde commented Feb 9, 2023 • edited Loading

kevinkreiser commented Feb 10, 2023

nilsnolde commented Feb 10, 2023

nilsnolde left a 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

nilsnolde Feb 9, 2023 •

edited

Loading

nilsnolde commented Feb 9, 2023 •

edited

Loading