Reduce code duplication in timedep forward/reverse #2987

danpat · 2021-04-08T05:40:13Z

Issue

PR No. 3 in the series towards #2885. This one tackles TimeDepForward/Reverse.

Main changes in this are:

Rename TimeDep to UnidirectionalAStar to be consistent with BidirectionalAStar
Templatize the behaviour and merge forward/reverse handling into a single unidirectionalastar.cc codebase.
Fix a bug in reverse routing that lead to incorrect routes, node counts and ETAs

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?

danpat · 2021-04-08T05:44:13Z

src/thor/timedep_forward.cc

-                                   const TimeInfo& time_info,
-                                   const valhalla::Location& destination,
-                                   std::pair<int32_t, float>& best_path) {
+template <TimeDepForward::ExpansionType expansion_direction, typename EdgeLabelT>


Note the EdgeLabelT - this is a temporary WIP trying to figure out how to handle EdgeLabel-vs-BDEdgeLabel.

danpat · 2021-04-08T05:46:18Z

src/thor/timedep_forward.cc

  }

  return disable_uturn;
 }

+template bool TimeDepForward::Expand<TimeDepForward::ExpansionType::reverse, BDEdgeLabel>(


Explicit declaration of reverse implementation so that the TimeDepReverse translation unit can find the template instantiation at link time. As the code stands, it's a bit weird to have a derived class trying to use template methods from the parent TU - I expect to refactor this detail away as I remove more code.

danpat · 2021-04-08T05:47:06Z

src/thor/timedep_forward.cc

+                                        std::pair<int32_t, float>& best_path) {
+  constexpr bool FORWARD = expansion_direction == TimeDepForward::ExpansionType::forward;
+
+  if (!FORWARD) {


Question to readers: conceptually why is this early-exit only done for the reverse search? Is this a copypasta bug, or is there some rationale?

cc @kevinkreiser @dnesbitt61

Code has changed considerably since I left it, but in reverse searches I initially added an early exit based on the directed edge's reverseaccess to avoid having to get the opposing directed edge and perform the more complete costing Allowed check. I believe I did some benchmarking around this and found a slight performance improvement. I can't comment on why shortcut edges are skipped.

danpat · 2021-04-08T05:48:13Z

src/thor/timedep_forward.cc

+    if (t2 == nullptr) {
+      return false;
+    }
+    opp_edge_id = t2->GetOpposingEdgeId(meta.edge);


Note: I renamed these from TimeDepReverse, they were oppedge and opp_edge there, which is a bit of a dubious naming scheme.

yea, opp_edge_id is a better choice!

danpat · 2021-04-08T05:49:00Z

src/thor/timedep_forward.cc

-                           time_info.local_time, nodeinfo->timezone())) {
-    return false;
+  if (FORWARD) {
+    if (meta.edge->is_shortcut() ||


Why is the forward search wrapped in is_shortcut(), but the reverse is not?

danpat · 2021-04-08T05:50:02Z

src/thor/timedep_forward.cc

+  if (FORWARD) {
+    newcost += transition_cost;
+  } else {
+    newcost.cost += transition_cost.cost;


This was similar to the thing found in the Dijkstra refactor - why does reverse only add cost? This seems like a bug, as it means a TimeDepReverse path won't accumulate time?

danpat · 2021-04-08T05:51:09Z

src/thor/timedep_forward.cc

-                                                                         : edgelabels_.size();
+      best_path.first = (meta.edge_status->set() == EdgeSet::kTemporary)
+                            ? meta.edge_status->index()
+                            : (FORWARD ? edgelabels_.size() : edgelabels_.size());


Note: these used to be different (edgelabels_rev_ and edgelabels_) - I've left this in for now as a reminder that forward/reverse behaviour may be specialized here.

danpat · 2021-04-08T05:51:52Z

src/thor/timedep_forward.cc

-    EdgeLabel& lab = edgelabels_[meta.edge_status->index()];
+    // TODO(danpat): can we slices down to EdgeLabel here safely?
+    EdgeLabel& lab =
+        FORWARD ? edgelabels_[meta.edge_status->index()] : edgelabels_[meta.edge_status->index()];


Similar to above - previously had different EdgeLabel/BDEdgeLabel for forward/reverse - left this in as a temporary reminder until I confirm it's OK to use BDEdgeLabel in all cases.

danpat · 2021-04-08T05:52:10Z

src/thor/timedep_forward.cc

    if (newcost.cost < lab.cost().cost) {
      float newsortcost = lab.sortcost() - (lab.cost().cost - newcost.cost);
-      adjacencylist_.decrease(meta.edge_status->index(), newsortcost);
+      if (FORWARD) {


And one more instance of the same.

danpat · 2021-04-08T05:53:25Z

src/thor/timedep_reverse.cc

@@ -54,245 +33,20 @@ void TimeDepReverse::Init(const midgard::PointLL& origll, const midgard::PointLL
  // Reserve size for edge labels - do this here rather than in constructor so
  // to limit how much extra memory is used for persistent objects.
  // TODO - reserve based on estimate based on distance and route type.
-  edgelabels_rev_.reserve(kInitialEdgeLabelCount);
+  edgelabels_.reserve(kInitialEdgeLabelCount);


This whole Init() thing can likely go away if we can use the BDEdgeLabel object for the forward search.

kevinkreiser · 2021-04-08T13:10:52Z

@danpat as an anecdote i can say that i semi recently refactored the forward direction of isochrones to use BDEdgeLabel. I seem to remember losing about 10% performance but I clawed it back with optimization to other parts of the code. I would much prefer the simplicity of using BDEdgeLabel everywhere (multimodal is another question mark though) if we can, so its definitely worth doing and measuring to see if its palatable.

…h template parameters - TODO: segfaulting sometimes, so this is incomplete.

…gfault.

…e-time branches.

danpat · 2021-04-14T23:08:20Z

Benchmark results. Switching to BDEdgeLabel for both doesn't seem to have a noticeable effect:

Invocation:

./bench/thor/benchmark-routes  --benchmark_min_time=3 --benchmark_filter=BM_GlobalFixedRandom --benchmark_counters_tabular=true --benchmark_repetitions=3 --planet-path=../../data/planet_2021_03_27-03_00_00.tar

master

-----------------------------------------------------------------------------------------------------------
Benchmark                                                      Time             CPU   Iterations     Routes
-----------------------------------------------------------------------------------------------------------
BM_GlobalFixedRandom<thor::TimeDepForward>                 14518 ms        14502 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepForward>                 15051 ms        15031 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepForward>                 15286 ms        15270 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepForward>_mean            14952 ms        14934 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepForward>_median          15051 ms        15031 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepForward>_stddev            394 ms          393 ms            3          0
BM_GlobalFixedRandom<thor::TimeDepReverse>                  7616 ms         7612 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepReverse>                  7364 ms         7361 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepReverse>                  7690 ms         7685 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepReverse>_mean             7556 ms         7553 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepReverse>_median           7616 ms         7612 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepReverse>_stddev            171 ms          170 ms            3          0
BM_GlobalFixedRandom<thor::BidirectionalAStar>             10702 ms        10685 ms            1          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>              9951 ms         9944 ms            1          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>              9918 ms         9912 ms            1          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>_mean        10190 ms        10180 ms            3          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>_median       9951 ms         9944 ms            3          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>_stddev        443 ms          437 ms            3          0

this branch

-----------------------------------------------------------------------------------------------------------
Benchmark                                                      Time             CPU   Iterations     Routes
-----------------------------------------------------------------------------------------------------------
BM_GlobalFixedRandom<thor::TimeDepForward>                 14716 ms        14689 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepForward>                 14435 ms        14423 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepForward>                 14335 ms        14324 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepForward>_mean            14495 ms        14479 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepForward>_median          14435 ms        14423 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepForward>_stddev            197 ms          189 ms            3          0
BM_GlobalFixedRandom<thor::TimeDepReverse>                  7445 ms         7437 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepReverse>                  7602 ms         7598 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepReverse>                  7427 ms         7424 ms            1          6
BM_GlobalFixedRandom<thor::TimeDepReverse>_mean             7491 ms         7486 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepReverse>_median           7445 ms         7437 ms            3          6
BM_GlobalFixedRandom<thor::TimeDepReverse>_stddev           96.4 ms         96.7 ms            3          0
BM_GlobalFixedRandom<thor::BidirectionalAStar>              9566 ms         9556 ms            1          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>              9578 ms         9575 ms            1          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>              9558 ms         9554 ms            1          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>_mean         9567 ms         9562 ms            3          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>_median       9566 ms         9556 ms            3          6
BM_GlobalFixedRandom<thor::BidirectionalAStar>_stddev       10.1 ms         11.8 ms            3          0

kevinkreiser · 2021-04-14T23:21:03Z

@danpat good news that the larger edgelabels didnt bother it. some of the results are suprisingly noisey in these benches though. ive been noticing it over the last few PRs. the standard deviation for example is off by orders of magnitude between runs sometimes. wtf is with that? im still so skeptical of gbench hahaha 😄

…ger-scale testing.

danpat · 2021-04-14T23:41:17Z

@kevinkreiser this bench running a small handful of quite slow routes - e.g. in the Forward test, 6 routes are calculated and it takes ~14 seconds. Given the duration of the individual test, I'm not all that surprised to see a pretty wobbly stddev, anything else happening on the box over that single timed 14 second run could interfere.

A better test might be to divide it up into smaller faster chunks - more iterations of a faster test would probably give better signal.

valhalla/thor/unidirectional_astar.h

kevinkreiser · 2021-04-21T14:34:04Z

looks good to me save for those two nit picks

danpat · 2021-04-21T15:02:19Z

@kevinkreiser yeah, not quite up-to-date with master.

I'm still digging into two curiosities before I mark this good:

https://github.com/valhalla/valhalla/blob/danpat_timedep_refactor_1/src/thor/unidirectional_astar.cc#L241-L247

I don't understand why reverse is only adding .cost here for transitions, rather than the whole thing.

The second, more concerning issue is this:

https://github.com/valhalla/valhalla/blob/danpat_timedep_refactor_1/test/gurka/test_time_tracking.cc#L265-L269

Even on a basic, unambiguous test, TimeDepReverse is returning different durations from node-to-node - IMO, this test should return identical results to the forward search version. It's also returning an extra node as you noted in the TODO there, so I think there's some off-by-one or missing termination condition somewhere in the reverse version of things.

So far, what I know is that it seems that the reverse search is continuing for one extra expansion after arriving at the origin - I suspect it might be a 0-cost u-turn or something.

This bug exists on master so I'm not going to kill myself trying to track it down, but I would like to clean up that cost adding thing.

kevinkreiser · 2021-04-21T15:11:38Z

Ah yes, when i wrote a bunch of the test all algorithms tests I had to disable some because it wasnt adding up properly. It seems you are finding the same thing. I didnt have a chance to dive into it at the time but it will be good to get to the bottom of it!

danpat · 2021-04-22T00:20:56Z

Got to the bottom of it - two problems in the reverse search:

SetOrigin wasn't swapping looking at begin_node/end_node like it should:

valhalla/src/thor/timedep_reverse.cc

Line 432 in a796844

has_other_edges = has_other_edges || !e.begin_node();

and

valhalla/src/thor/timedep_reverse.cc

Line 555 in a796844

if (has_other_edges && edge.begin_node()) {

This was the source of the extra node at the start of the path.
FormPath in reverse mode was failing to subtract the current transition cost when it was adding the previous transition cost:

valhalla/src/thor/timedep_reverse.cc

Line 603 in a796844

cost += previous_transition_cost;

This was the source of the differing timing information on the reverse path compared to the forward search.

With these two fixes, forward/reverse now give consistent results. Commits coming shortly once I verify all the other tests.

UPDATE: c9caf70 contains the fix

…lla into danpat_timedep_refactor_1

test/gurka/test_search_filter.cc

…perly.

danpat · 2021-04-22T05:40:35Z

@kevinkreiser I think I'm good with the current state of this - 350 fewer lines of code and a couple of bugs fixed, .

src/thor/unidirectional_astar.cc

…lla into danpat_timedep_refactor_1

src/thor/unidirectional_astar.cc

mandeepsandhu

LGTM. Thanks for this big cleanup (and squashed an imp bug too!) 🙏

Move expand/expandinner into single template functions.

c620817

danpat commented Apr 8, 2021

View reviewed changes

Lint fix

c4d1350

danpat added 13 commits April 8, 2021 16:56

Start to consolidate into a single class for forward/reverse.

7d61977

Collapse SetOrigin so we only have one version of that for timedep.

3d933c2

Consolidate SetDestinatino and FormPath.

c4c77ae

Combine timedep_reverse/timedep_forward into unidirectional_astar wit…

91a7b22

…h template parameters - TODO: segfaulting sometimes, so this is incomplete.

Don't shadow 'tile' variable and leave it uninitialized leading to se…

72fb6df

…gfault.

Fix formatting.

982c572

fix missing rename

4d7cb50

Merge branch 'master' into danpat_timedep_refactor_1

12a7251

Don't need template specializations for GetBestPath, just some compil…

a3e8f3f

…e-time branches.

Fix formatting.

7f6f38c

Some more tidy up.

bc3fd45

Merge branch 'master' into danpat_timedep_refactor_1

22bb76b

More cleanup

4df7ddf

Update route benchmark so it can be run against a planet file for lar…

9090230

…ger-scale testing.

kevinkreiser reviewed Apr 21, 2021

View reviewed changes

valhalla/thor/unidirectional_astar.h Outdated Show resolved Hide resolved

Merge branch 'master' into danpat_timedep_refactor_1

07d5410

danpat added 5 commits April 21, 2021 17:34

Simple cleanup.

cd0cf82

Fix timedep reverse search termination and costing summing.

c9caf70

Merge branch 'danpat_timedep_refactor_1' of github.com:valhalla/valha…

44e31c5

…lla into danpat_timedep_refactor_1

Merge branch 'master' into danpat_timedep_refactor_1

8f77691

Update changelog.

946fc74

mandeepsandhu reviewed Apr 22, 2021

View reviewed changes

test/gurka/test_search_filter.cc Show resolved Hide resolved

danpat added 3 commits April 21, 2021 22:01

Use a single Expansiontype enum across all refactored code.

e84cecf

Review cleanup.

0189b05

Swap order in assert so that if it fails, the error message reads pro…

9f16f46

…perly.

danpat added 2 commits April 22, 2021 07:26

Merge branch 'master' into danpat_timedep_refactor_1

ef74b6e

Merge branch 'master' into danpat_timedep_refactor_1

6f3f3d2

mandeepsandhu reviewed Apr 23, 2021

View reviewed changes

src/thor/unidirectional_astar.cc Outdated Show resolved Hide resolved

danpat added 2 commits April 22, 2021 22:28

Correct some stale comments.

c9206f6

Merge branch 'danpat_timedep_refactor_1' of github.com:valhalla/valha…

4836e68

…lla into danpat_timedep_refactor_1

mandeepsandhu reviewed Apr 23, 2021

View reviewed changes

src/thor/unidirectional_astar.cc Outdated Show resolved Hide resolved

mandeepsandhu previously approved these changes Apr 23, 2021

View reviewed changes

Cleanup.

e98cc1e

danpat dismissed mandeepsandhu’s stale review via e98cc1e April 23, 2021 13:21

purew approved these changes Apr 23, 2021

View reviewed changes

danpat merged commit 9fe3c6a into master Apr 23, 2021

danpat deleted the danpat_timedep_refactor_1 branch April 23, 2021 16:47

This was referenced Apr 26, 2021

handling time dependent restrictions #3029

Merged

Incorrect path edges if location snaps to a node #2940

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce code duplication in timedep forward/reverse #2987

Reduce code duplication in timedep forward/reverse #2987

danpat commented Apr 8, 2021 •

edited

Loading

danpat Apr 8, 2021

danpat Apr 8, 2021

danpat Apr 8, 2021 •

edited

Loading

dnesbitt61 Apr 8, 2021

danpat Apr 8, 2021

dnesbitt61 Apr 8, 2021

danpat Apr 8, 2021

danpat Apr 8, 2021

danpat Apr 8, 2021

danpat Apr 8, 2021

danpat Apr 8, 2021

danpat Apr 8, 2021

kevinkreiser commented Apr 8, 2021 •

edited

Loading

danpat commented Apr 14, 2021 •

edited

Loading

kevinkreiser commented Apr 14, 2021

danpat commented Apr 14, 2021

kevinkreiser commented Apr 21, 2021

danpat commented Apr 21, 2021 •

edited

Loading

kevinkreiser commented Apr 21, 2021

danpat commented Apr 22, 2021 •

edited

Loading

danpat commented Apr 22, 2021 •

edited

Loading

mandeepsandhu left a comment

Reduce code duplication in timedep forward/reverse #2987

Reduce code duplication in timedep forward/reverse #2987

Conversation

danpat commented Apr 8, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpat Apr 8, 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

kevinkreiser commented Apr 8, 2021 • edited Loading

danpat commented Apr 14, 2021 • edited Loading

master

this branch

kevinkreiser commented Apr 14, 2021

danpat commented Apr 14, 2021

kevinkreiser commented Apr 21, 2021

danpat commented Apr 21, 2021 • edited Loading

kevinkreiser commented Apr 21, 2021

danpat commented Apr 22, 2021 • edited Loading

danpat commented Apr 22, 2021 • edited Loading

mandeepsandhu left a comment

Choose a reason for hiding this comment

danpat commented Apr 8, 2021 •

edited

Loading

danpat Apr 8, 2021 •

edited

Loading

kevinkreiser commented Apr 8, 2021 •

edited

Loading

danpat commented Apr 14, 2021 •

edited

Loading

danpat commented Apr 21, 2021 •

edited

Loading

danpat commented Apr 22, 2021 •

edited

Loading

danpat commented Apr 22, 2021 •

edited

Loading