Check for complex restriction where bidirectional expansion meets #2242

purew · 2020-02-17T16:18:56Z

Issue

This PR adds an improved check of complex restrictions on the bridging edge that connects the two search directions in Bidirectional astar.

See linked issue for more detailed information.

Fixes #2228

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Generally use squash merge to rebase and clean comments before merging
Update the changelog

Requirements / Relations

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

Adds jönköping tiles for complex restrictions test on short paths Prevents parallel runs of valhalla_build_timezones Adds lock around valhalla_build_timezones Adds test with complex restriction where bidirectional meet The edge that connects the two directions of expansion in Bidirectional is dropped if it is part of a complex restriction. The correct behaviour is instead to walk the edges back and forth to see if any complex restriction really is active. References #2228 Resolves added transition_cost from previous merge from master WIP `IsBridgingEdgeRestricted` Implements IsBridgingEdgeRestricted in forward direction Adds tests for complex restrictions for `IsBridgingEdgeRestricted` Adds failing test with BidirectionalAStarSingleDirection Back to BidirectionalAstar, test fails without forwardexpansion Implements IsBridgingEdgeRestricted in reverse direction WIP: Realises it is harder than expected to check opposing tree of expansion Adds complexrestriction::CheckPatchPathForRestrictions which checks a patch-path against a list of restrictions Added unittest test_IsBridgingEdgeRestricted which passes all astar tests pass Remove couts Restored DynamicCost::Restricted api More comment Removes dead code More cleanup of unneeded changes Try different protobuf api CircleCI has old protobuf Cleaning up Tiles with greg astar tests passing clean up. added reverse logic for the restriction.

…-short-paths-clean-3

[ FAILED ] Astar.test_complex_restriction_short_path_melborne [ FAILED ] Astar.test_IsBridgingEdgeRestricted

…All tests pass

… for edges

…-short-paths-clean-3

kevinkreiser · 2020-02-17T18:54:03Z

src/thor/bidirectional_astar.cc

+    patch_path.push_back(edgeid);
+
+    // Also grab restrictions while walking for later comparison against patch_path
+    //


extra comment

kevinkreiser · 2020-02-17T18:55:52Z

src/thor/bidirectional_astar.cc

+    //
+    tile = graphreader.GetGraphTile(edgeid, tile);
+    if (tile == nullptr) {
+      throw std::logic_error("Tile pointer was null in IsBridgingEdgeRestricted");


this just cant happen, because the edge is coming from the path that the algorithm was expanding

I think it is fine to have some kind of check around "impossible" states. Would you prefer to log an error message or some other effect?

kevinkreiser · 2020-02-17T18:56:17Z

src/thor/bidirectional_astar.cc

+    }
+    const auto& edge = tile->directededge(edgeid);
+    if (edge == nullptr) {
+      throw std::logic_error("Edge pointer was null in IsBridgingEdgeRestricted");


kevinkreiser · 2020-02-17T18:59:21Z

src/thor/bidirectional_astar.cc

+      break;
+    }
+    next_rev_pred = edge_labels_rev[next_rev_pred_idx];
+    if (!next_rev_pred.on_complex_rest()) {


i remember mentioning about breaking early when we reviewed this a bit ago. was this the change to do that. for some reason i thought that we needed the tile to do the check but honeslty my memory is fading

The edge carries the bit describing the existence of any complex restriction. We probably conflated this with checking for the full set of matching restrictions from tile->GetRestrictions.

kevinkreiser

looks good just some small comments and questions following up from the last review we did

due to x86 32bit precision

purew · 2020-02-24T18:45:01Z

test/astar.cc

  }
-  auto correct_len = 5;
-  ASSERT_EQ(paths.size(), correct_len) << "Wrong number of paths in response";
+  //{


Hmm this isn't supposed to be commented.

…tion

… label, fix up a* fake tile setup

kevinkreiser · 2020-02-25T22:32:24Z

This PR is just waiting on RAD testing at the moment. I'll do some rounds of that tomorrow to verify the differences. Otherwise this is ready to ship.

purew · 2020-02-28T21:55:04Z

I ran RAD tests on a common set of routes with no diff to master.

Running a second set of RAD on a complex restriction dense area shows good improvements in time and length of routes.

* master: Post merge feedback on #2253. (#2257) Check for complex restriction where bidirectional expansion meets (#2242) Update README.md

…populate the segments * master: Transition matching (#2258) Post merge feedback on #2253. (#2257) Check for complex restriction where bidirectional expansion meets (#2242) Update README.md Configurable load testing of /route and /trace_route endpoints (via wrk) (#2253) Polyline Resampling Addition and Bug Fix (#2239) Update exit logic for non-motorways (#2252) Add app. to get a list of elevation tiles that are covered within the (#2248) Fix RapidJSON warnings and naming conflicts (#2249) Add basic truck costing documentation (#2241) # Conflicts: # test/mapmatch.cc

purew added 14 commits February 14, 2020 11:10

Merge remote-tracking branch 'origin/master' into complex-restriction…

6291758

…-short-paths-clean-3

leftover from merge

ef3196d

Adds end-edge from restriction

72c6ff8

test_IsBridgingEdgeRestricted and australia fails

674fc95

[ FAILED ] Astar.test_complex_restriction_short_path_melborne [ FAILED ] Astar.test_IsBridgingEdgeRestricted

IsBridgingEdgeRestricted grabs restrictions from opposite direction. …

6ce6846

…All tests pass

Simplifies IsBridgingEdgeRestricted by only running in forward direction

c0b9995

Revert changes to valhalla_build_timezones

049d6f5

Expands astar tileset and restriction and fixes missing opp_local_idx…

2e89350

… for edges

Removes unneeded CheckPatchPathForRestrictions

08c9cda

nit: Hinting for GetGraphTile

eb075d6

Single melborne from greg, noroute on melborne test

61659a9

Adds second test-case, of a X-crossing

c209640

Renumbers melborne pbf

f5b47c2

purew requested a review from kevinkreiser February 17, 2020 16:18

purew added 3 commits February 17, 2020 11:20

Updates readme

032b5df

Merge remote-tracking branch 'origin/master' into complex-restriction…

bc6f79e

…-short-paths-clean-3

Removes flock, (not on osx)

5533e4a

kevinkreiser reviewed Feb 17, 2020

View reviewed changes

kevinkreiser previously approved these changes Feb 17, 2020

View reviewed changes

Merge branch 'master' into complex-restriction-short-paths-clean-3

52687a8

purew dismissed kevinkreiser’s stale review via 52687a8 February 19, 2020 21:12

purew added 4 commits February 19, 2020 16:24

Nit cleanup comment

87d7777

bump ci

3f5cb7f

Merge branch 'master' into complex-restriction-short-paths-clean-3

58c14ba

test/summary: Verifies shape of path and losens EXPECT_NEAR

b0cb1e6

due to x86 32bit precision

purew force-pushed the complex-restriction-short-paths-clean-3 branch from 9ea5709 to b0cb1e6 Compare February 21, 2020 15:42

gknisely and others added 4 commits February 21, 2020 17:06

add admin db for netherlands.

95ee56a

updated based on drive on right

1845319

format

99f7deb

WIP Changes test/summary path

b8ce8f4

purew commented Feb 24, 2020

View reviewed changes

purew and others added 6 commits February 24, 2020 14:35

Enables commented test and fixes complex restriction in reverse direc…

5006497

…tion

fix summary test

4bb0bee

fix one test

3397053

update durations for admin enhanced utrecht

52a6c86

Merge branch 'master' into complex-restriction-short-paths-clean-3

9a32b64

fix comment, include start and end in the restriction bit on the edge…

bbd9c9c

… label, fix up a* fake tile setup

purew and others added 2 commits February 27, 2020 19:18

Fixes faulty fwd_pred

85dc5bf

Merge branch 'master' into complex-restriction-short-paths-clean-3

8f59fd8

kevinkreiser approved these changes Feb 28, 2020

View reviewed changes

Merge branch 'master' into complex-restriction-short-paths-clean-3

3cd0d31

purew merged commit 4559af9 into master Feb 29, 2020

yuzheyan added a commit that referenced this pull request Mar 2, 2020

Merge branch 'master' into transition_matching

35b84ee

* master: Post merge feedback on #2253. (#2257) Check for complex restriction where bidirectional expansion meets (#2242) Update README.md

purew deleted the complex-restriction-short-paths-clean-3 branch March 10, 2020 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for complex restriction where bidirectional expansion meets #2242

Check for complex restriction where bidirectional expansion meets #2242

purew commented Feb 17, 2020 •

edited

Loading

kevinkreiser Feb 17, 2020

kevinkreiser Feb 17, 2020

purew Feb 19, 2020

kevinkreiser Feb 17, 2020

kevinkreiser Feb 17, 2020

purew Feb 19, 2020

kevinkreiser left a comment

purew Feb 24, 2020

kevinkreiser commented Feb 25, 2020

purew commented Feb 28, 2020

Check for complex restriction where bidirectional expansion meets #2242

Check for complex restriction where bidirectional expansion meets #2242

Conversation

purew commented Feb 17, 2020 • edited Loading

Issue

Tasklist

Requirements / Relations

kevinkreiser Feb 17, 2020

Choose a reason for hiding this comment

kevinkreiser Feb 17, 2020

Choose a reason for hiding this comment

purew Feb 19, 2020

Choose a reason for hiding this comment

kevinkreiser Feb 17, 2020

Choose a reason for hiding this comment

kevinkreiser Feb 17, 2020

Choose a reason for hiding this comment

purew Feb 19, 2020

Choose a reason for hiding this comment

kevinkreiser left a comment

Choose a reason for hiding this comment

purew Feb 24, 2020

Choose a reason for hiding this comment

kevinkreiser commented Feb 25, 2020

purew commented Feb 28, 2020

purew commented Feb 17, 2020 •

edited

Loading