Increase limits for timedep_* algorithms to fix "no route found" #2845

genadz · 2021-02-08T18:50:09Z

Issue

the problem was described here #2834 . So, I did 2 things:

split track penalty into 2: edge factor and transition penalty. This change allowed to decrease track_factor (it boosted convergence) and the same time to keep avoid tracks.
found new limits for timedep_* algorithms to save "no routes" rate the same as before.

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

Requirements / Relations

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

genadz · 2021-02-08T18:54:45Z

on auto.txt routes (with time) performance decreased on ~5%.

kevinkreiser · 2021-02-08T19:31:49Z

src/sif/dynamiccost.cc

@@ -53,9 +53,12 @@ namespace sif {
 // default options/parameters
 namespace {

+// max penalty to apply when use tracks
+constexpr float kMaxTrackPenalty = 300.f; // 5 min


are you going to add a costing option to all the user to set this? a lot of our other penalties work like this like gate penalty and alley penalty

I've implemented the same logic as @SvetlanaBayarovich for living_street. reasons:

I couldn't come up with a usecase that can't be covered with single use_tracks parameter; so, as for me to have track_penalty as external parameter is superfluously.

it's clear separation: use_tracks - external parameter in range [0..1]; track_factor,track_penalty - internal parameters that are measured in internal units and can be tweaked by us without notifying external users.

kevinkreiser · 2021-02-08T19:33:02Z

src/thor/timedep_forward.cc

@@ -14,7 +14,7 @@ namespace thor {
 constexpr uint32_t kInitialEdgeLabelCount = 500000;

 // Number of iterations to allow with no convergence to the destination
-constexpr uint32_t kMaxIterationsWithoutConvergence = 800000;
+constexpr uint32_t kMaxIterationsWithoutConvergence = 1800000;


oof more than double :( i guess the good news is that we rarely reach this limit except for some extreme cases?

right, it happens not very often

kevinkreiser · 2021-02-08T19:34:28Z

test/gurka/test_use_tracks.cc

           |        |       |
-           C--------D       G


can you briefly describe the test change, its not really obvious from the diff

yeah. as far as I introduced track_penalty, route ABEF became cheaper and route ABCDEF became more expensive (because of high penalty in transition cost). So, in order to demonstrate track avoidance feature, we should have route completely without tracks as ABCDEF and with tracks as ABEF

kevinkreiser · 2021-02-08T19:34:59Z

this looks good to me. myy only question is about adding the penality as a parameter in the request like the other ones are

genadz · 2021-02-09T11:06:28Z

Performance test. I ran auto.txt requests with time parameter (arrive_by) on my local machine. Below is results I got:

set use_tracks=0.5 (do not use penalties for tracks)

percentiles	5%	10%	20%	50%	90%	95%	99%	100%
response_time	0.0s	0.0s	0.0s	0.2s	1.4s	2.1s	4.1s	21.7s

set use_tracks=0 (max penalties for track roads)

percentiles	5%	10%	20%	50%	90%	95%	99%	100%
response_time	0.0s	0.0s	0.0s	0.2s	1.4s	2.0s	3.7s	26.3s

as we can see, for 99% of routes timings remained almost the same (with current branch became even less, but probably this is because of measurement error). For most slowest routes performance decreased on ~18%
@kevinkreiser @kshehadeh
hm, one more note: in terms of iterations for some cases it's doubled, but we don't see double performance drop. I think it may be explained as follows: we have more errors "No convergence to destination after ... iterations" on master than on my branch. In that cases we rerun algorithm with relaxed limits and it allows to find the route (often). So, in other words we run algorithm twice when get "No convergence" error. It means, that for cases when we have such error for master and don't have for current branch - current branch could work faster because of only one run.
total performance on timedep routes decreased ~8% in terms of iterations

mandeepsandhu · 2021-02-09T20:53:19Z

src/sif/dynamiccost.cc

+  // Calculate penalty value based on use preference. Return value
+  // in range [kMaxTrackPenalty; 0], if use < 0.5; or
+  // 0, if use > 0.5.
+  track_penalty_ = use_tracks < 0.5f ? (kMaxTrackPenalty * (1.f - 2.f * use_tracks)) : 0.f;


Is this parameter documented somewhere? I couldn't find it in the API docs.

Its math for generating the track penalty is not very straight-forward. Eg: setting a value of 0.4 would mean applying a penalty of 1 min (?) to each track edge. CMIIW. Makes it a bit hard to reason as to what affect the value has without looking at the code.

Can we document this somewhere?

good catch, I forgot to add use_tracks option to docs( will do. the reasons why we use single option I described here #2845 (comment)

mandeepsandhu

Minor commetn about docs. Rest LGTM!

genadz · 2021-02-10T08:42:38Z

@kevinkreiser do you know why build-docker-images job failed ? I see the error Must provide --username with --password-stdin but don't understand why it happened, because I see that variables DOCKERHUB_PASS, DOCKERHUB_USERNAME exist in project settings

kevinkreiser · 2021-02-10T12:56:31Z

@genadz its because you forked the repository and you dont have the dockerhub credentials. i'l work on making that build robust to that. you can ignore it for now

kevinkreiser · 2021-02-10T12:58:33Z

should be fixed now

genadz · 2021-02-10T13:05:03Z

should be fixed now

thanks a lot!

genadz added 3 commits February 8, 2021 18:59

Split track_factor into edge factor and transition penalty

3d5b9c5

Increase iterations limit for timedep_* algorithm

61952fe

Update changelog

8f89469

genadz requested review from SvetlanaBayarovich and kevinkreiser February 8, 2021 18:52

kevinkreiser reviewed Feb 8, 2021

View reviewed changes

genadz requested review from kevinkreiser and mandeepsandhu February 9, 2021 11:49

kevinkreiser previously approved these changes Feb 9, 2021

View reviewed changes

Merge branch 'master' into split_track_factor

388917b

mandeepsandhu reviewed Feb 9, 2021

View reviewed changes

mandeepsandhu previously approved these changes Feb 9, 2021

View reviewed changes

Add 'use_tracks' option to docs

80613ae

genadz dismissed stale reviews from mandeepsandhu and kevinkreiser via 80613ae February 10, 2021 08:36

genadz requested a review from mandeepsandhu February 10, 2021 08:38

genadz requested a review from kevinkreiser February 10, 2021 08:42

Merge branch 'master' into split_track_factor

5f4e422

kevinkreiser approved these changes Feb 10, 2021

View reviewed changes

genadz merged commit 0dec3f7 into valhalla:master Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase limits for timedep_* algorithms to fix "no route found" #2845

Increase limits for timedep_* algorithms to fix "no route found" #2845

genadz commented Feb 8, 2021 •

edited

Loading

genadz commented Feb 8, 2021

kevinkreiser Feb 8, 2021

genadz Feb 9, 2021 •

edited

Loading

kevinkreiser Feb 8, 2021

genadz Feb 9, 2021

kevinkreiser Feb 8, 2021

genadz Feb 9, 2021

kevinkreiser commented Feb 8, 2021

genadz commented Feb 9, 2021 •

edited

Loading

mandeepsandhu Feb 9, 2021 •

edited

Loading

genadz Feb 10, 2021

mandeepsandhu left a comment

genadz commented Feb 10, 2021

kevinkreiser commented Feb 10, 2021

kevinkreiser commented Feb 10, 2021

genadz commented Feb 10, 2021

Increase limits for timedep_* algorithms to fix "no route found" #2845

Increase limits for timedep_* algorithms to fix "no route found" #2845

Conversation

genadz commented Feb 8, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

genadz commented Feb 8, 2021

kevinkreiser Feb 8, 2021

Choose a reason for hiding this comment

genadz Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

kevinkreiser Feb 8, 2021

Choose a reason for hiding this comment

genadz Feb 9, 2021

Choose a reason for hiding this comment

kevinkreiser Feb 8, 2021

Choose a reason for hiding this comment

genadz Feb 9, 2021

Choose a reason for hiding this comment

kevinkreiser commented Feb 8, 2021

genadz commented Feb 9, 2021 • edited Loading

mandeepsandhu Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

genadz Feb 10, 2021

Choose a reason for hiding this comment

mandeepsandhu left a comment

Choose a reason for hiding this comment

genadz commented Feb 10, 2021

kevinkreiser commented Feb 10, 2021

kevinkreiser commented Feb 10, 2021

genadz commented Feb 10, 2021

genadz commented Feb 8, 2021 •

edited

Loading

genadz Feb 9, 2021 •

edited

Loading

genadz commented Feb 9, 2021 •

edited

Loading

mandeepsandhu Feb 9, 2021 •

edited

Loading