Fix NoRoute for waypoints with low reachability edges #2914

genadz · 2021-03-04T21:27:42Z

Issue

The main idea and unittest described here #2872 . This PR fixes problem with "no route found" for multipoint requests with intermediate points on isolated (unreachable) islands. In case of fail, we remove all low reachable edge candidates for all intermediate waypoints. It forces routing algorithm to use edges from large network that should be the same with a high probability.

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?

kevinkreiser · 2021-03-05T02:47:14Z

valhalla/loki/search.h

@@ -13,22 +13,27 @@
 namespace valhalla {
 namespace loki {

+struct SearchOptions {
+  uint32_t max_reachability_ = 100;


why do we need this and why do we need to store an extra bool on the candidates we find? each location already has the max_reach assigned to it (minimum_reachabilty). cant we just add a member to that class that says:

bool high_reach() const { return outbound >= min_reach && inbound >= min_reach; }

I fail to see why its needed to keep this information and really make any of the changes in loki search when the candidate is part of the location which already knows what the max reach could be and the candidate knows what the actual reach was.

yeah, I also don't like this too much. but, current solutions assumes that we detect big connectivity components. so, if we got location with already set minimum_reachability value that is actually low, in this case high_reach() method will return true even in cases when this edge lies in a small component.

hm, so, probably for route requests we should override minimum_reachability with the max_reachability value from the config

@kevinkreiser outbound, inbound values are stored in PathEdge class but minumun_reachability is kept in PathLocation. there are 2 possible solutions I see:

define bool high_reach(uint32_t min_inbound_reach, uint32_t min_outbound_reach) const method in PathEdge class and use min_outbound_reach_,min_inbound_reach_ values from Location class to pass to this method;

define bool high_reach(const PathEdge& path_edge) const method in PathLocation class and use it to detect if an edge is high reachable or not

yep either of those would be good, that is what i was thinking too 👍

either way we have to make an external method (one of the major draw backs of protobuf) but i think we can do this inside of thor whereever we need this information for edge culling

src/thor/route_action.cc

…stead

…bility.

genadz · 2021-03-09T10:43:58Z

@kevinkreiser as expected, performance hasn't changed.

merkispavel

I looked through the changes. At first sight I haven't found any bugs but I'll leave 🚢 to be Kevin's responsibility ;)
p.s seeing this code for the first time, so I might miss some details

* there are 2 types of routes: 1) with middle points on oneway deadends; 2) points sutiated in different small connectivity components. We should provide some paths instead of "NoRoute" errors that we have right now.

genadz · 2021-03-10T15:14:19Z

test_requests/through_routes.txt

+-j '{"costing":"auto","locations":[{"lat":38.245509,"lon":-85.75095,"type":"break"},{"lat":38.251,"lon":-85.763,"type":"through"},{"lat":38.258075,"lon":-85.744407,"type":"break"}]}'
+-j '{"costing":"auto","locations":[{"lat":52.027886,"lon":5.077448,"type":"break"},{"lat":52.030559,"lon":5.078592,"type":"through"},{"lat":52.030178,"lon":5.080847,"type":"through"},{"lat":52.025977,"lon":5.08614,"type":"break"}]}'
+-j '{"costing":"auto","locations":[{"lat":39.821226,"lon":-84.317642,"type":"break"},{"lat":39.828069,"lon":-84.314242,"type":"through"},{"lat":39.832658,"lon":-84.318762,"type":"through"},{"lat":39.828843,"lon":-84.323137,"type":"break"}]}


test routes that fail right now

kevinkreiser · 2021-03-11T14:23:11Z

src/baldr/pathlocation.cc

@@ -61,5 +61,9 @@ bool PathLocation::shares_edges(const PathLocation& other) const {
  return true;
 }

+bool PathLocation::is_high_reachable(const PathEdge& edge) const {


nit: should be is_highly_reachable or has_high_reach

kevinkreiser · 2021-03-11T14:23:28Z

src/loki/search.cc

@@ -407,7 +407,7 @@ struct bin_handler_t {
      auto opposing_edge_id = reader.GetOpposingEdgeId(candidate.edge_id, other_edge, other_tile);

      if (other_edge && costing->Allowed(other_edge, other_tile)) {
-        auto reach = get_reach(opposing_edge_id, other_edge);


hahaha oops! thanks for removing the shadowing!

kevinkreiser · 2021-03-11T14:29:53Z

test/gurka/test_reachability.cc

+    // Should give the same result as A->1->C
+    auto result = gurka::do_action(valhalla::Options::route, map, {"A", "D", "C"}, "auto", {}, {},
+                                   nullptr, "break_through");
+    gurka::assert::raw::expect_path(result, {"AB", "AB", "AB", "BC"});


why are there three AB edges? im sure there is some detail i just cant remember at the moment... i get why there is one from A to the snap point at or near 1 and that then there is another for the next leg from that point to B but i cant figure out where the 3rd one is coming from at the moment. can you jog my memory?

ha, great question!) actually, the first AB edge is reversed. I think there is a small bug in TimeDepForward algorithm. I debugged it, there is a combination of conditions in TimeDepForward::SetOrigin method that led to this:

looks like we should remove all inbound edges in case of the origin was snapped to the node, but we didn't do ti because of this https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L432 : origin and destination is on the same edge;

when we got forward path edge AB we found that the destination also on this edge, and added forward AB edge to the priority queue with cost equal to A -> 1, BUT didn't settle this edge because of this comment https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L530 ;

last step, we have edges ABr and ABf in the queue and sortcost for ABr edge is less than sortcost for ABf. So, pop edge ABr from the queue, start expansion from the node A and found destination edge ABf. The end)

I think, this should be fixed in another PR.

interesting, we have a bit of code that is supposed to do this if i can remember where it is. it should say somethign like if its node snapped and its an origin no point in using the edge that is snapped to its end node...

this bit of code: https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L424-L429

the purpose is to ignore end node snaps for origins of the route unless that is all we have

https://github.com/valhalla/valhalla/blob/master/src/thor/timedep_forward.cc#L432 - we also use this condition that just checks that origin and destination on the same edge. so , probably it's needless

kevinkreiser · 2021-03-11T14:30:41Z

src/thor/route_action.cc

-    auto destination = std::prev(origin);
-    thor::PathAlgorithm* path_algorithm =
-        get_path_algorithm(costing, *origin, *destination, api.options());
+    thor::PathAlgorithm* path_algorithm = get_path_algorithm(costing, *origin, *destination, options);
    path_algorithm->Clear();
    algorithms.push_back(path_algorithm->name());
    LOG_INFO(std::string("algorithm::") + path_algorithm->name());

    // TODO: delete this and send all cases to the function above


so since we deleted this, then can we delete that function i had lying around above and also delete this comment?

kevinkreiser · 2021-03-11T14:31:19Z

src/thor/route_action.cc

+  }
+  loc.mutable_filtered_edges()->Clear();
+}
+
 /**
 // removes any edges from the location that aren't connected to it (because of radius)
 void remove_edges(const GraphId& edge_id, valhalla::Location& loc, GraphReader& reader) {


delete this function i mean, since you are doing it a different way

kevinkreiser · 2021-03-11T14:33:16Z

valhalla/baldr/pathlocation.h

+  /**
+   * Check if path edge has high reachability for this location.
+   * @param edge PathEdge that should be checked
+   * @return     true if the edge is high reachable
+   */
+  bool is_high_reachable(const PathEdge& edge) const;


seems this method is actually unused. we instead use the free function inside thor route_action

will remove, thanks

src/thor/route_action.cc

kevinkreiser · 2021-03-11T14:48:10Z

src/thor/route_action.cc

+        // reset state and try to route again
+        route = nullptr;
+        first_edge = {};
+        vias.clear();
+        path.clear();
+        algorithms.clear();
+        trip.mutable_routes()->Clear();
+        origin = ++correlated.rbegin();
+        continue;


resets the entire state of all the legs of the route and starts completely over from the beginning doing all the legs over.

is it proposition for comment ? to make more detailed ?

It's just me clarifying my understanding or anyone else's who is following along haha

kevinkreiser · 2021-03-11T14:50:17Z

src/thor/route_action.cc

+      // if routing failed because an intermediate waypoint was snapped to the low reachability road
+      // (such road lies in a small connectivity component that is not connected to other locations)
+      // we should leave only high reachability candidates and try to route again
+      if (allow_retry && is_through_point(*destination) &&


cant this happen on other locations types than through though? like even the break or via types could fail right?

for break and via types we use all path edges. it means if we failed to find a route it's definitely not a connectivity problem, so it doesn't make sense to remove low reachable edges and retry

haha i made the mistake of leaving this comment on the arrive_by which is harder to think about. let me see if i can mentally find a crack in the forward direction (depart_at). i just need to mull it over a bit to see if it handles everything we're worried about sorry for the confusion, just want to really understand the changes to this critical section ;)

kevinkreiser · 2021-03-11T14:51:17Z

src/thor/route_action.cc

+      // (such road lies in a small connectivity component that is not connected to other locations)
+      // we should leave only high reachability candidates and try to route again
+      if (allow_retry && is_through_point(*destination) &&
+          !is_high_reachable(*destination, destination->path_edges(0))) {


the reason why 0 works here is only because its a through which means we've culled all other edges but the matching ones? what about on the first leg of the route where we havent dont any culling yet, why does 0 always work here?

hm, good question. I though that the first (last) point can't be a through type

youre right it cant!

should we handle it somehow ?

genadz · 2021-03-11T18:54:19Z

src/thor/route_action.cc

+      if (allow_retry && is_through_point(*origin) &&
+          !is_high_reachable(*origin, origin->path_edges(0))) {


@kevinkreiser I think such condition

if (allow_retry && origin != correlated.begin() && is_through_point(*origin) && origin->path_edges_size() > 0 && !is_highly_reachable(*origin, origin->path_edges(0)))

should handle all cases

* move all logic about removing path edges into one general function; * fixed naming: is_high_reachable -> is_highly_reachable; * made comment about resetting state more detailed (thanks Kevin);

kevinkreiser · 2021-03-12T14:45:09Z

src/thor/route_action.cc

+      if (allow_retry && origin != correlated.begin() && is_through_point(*origin) &&
+          origin->path_edges_size() > 0 && !is_highly_reachable(*origin, origin->path_edges(0))) {


ok so we dont have to solve this in this PR but... it is possible that the router will use a different edge at the end of one leg than the beginning of the next one. so while you were right that we only need to currently care about handling a failure for through locations (because only they widdle down the edges to just the one that was used in the adjacent leg) it is the case that the router could have a situation like i described in my previous comment.

lets go with the simple case: break->break->break

assume that the middle break point has two pairs of edge candidates one inside a small component and one outside. assume also that the first break point is inside the small component. what happens is then the fist leg is in the component and the second leg is outside of it. makign the returned route, invalid. so you are right, it wont fail the route but it will return something unusable to the user. this is a long standing issue and a rare occurance, but i wanted to point it out to you here.

we probably should be removing some of the pairs of candidates even for breaks and vias if they arent directly connected to the last edge in the previous edge. thats i think what i planned to do with that commented out function above but it was so long ago i dont quite remember ))

it's very useful comment, thank you! we should rethink this logic and fix in a separate issue. I'll crate after this one gets merged

kevinkreiser · 2021-03-12T14:47:03Z

src/thor/route_action.cc

-    auto temp_paths = get_path(path_algorithm, *origin, *destination, costing, api.options());
+    auto temp_paths = get_path(path_algorithm, *origin, *destination, costing, options);
+    if (temp_paths.empty())
+      return false;


nit: this line isnt covered because your unit test doesnt use date_time type=2 (arrive_by). you could increase coverage by adding another permutation just in case to make sure that the fix works for arrive_by as well.

kevinkreiser · 2021-03-12T14:47:28Z

src/thor/route_action.cc

+            // no route found
+            throw valhalla_exception_t{442};
+        }
+        // resets the entire state of all the legs of the route and starts completely


all the coverage checks above are the same thing, if you add an arrive_by route that needs a retry then these will all be resolved

kevinkreiser · 2021-03-12T14:49:29Z

test/gurka/test_reachability.cc

@@ -0,0 +1,51 @@
+#include "gurka.h"


nit: there is already a test_reach and a test_route gurka test, maybe this should just go in one of those?

genadz · 2021-03-15T12:01:49Z

@kevinkreiser updates tests to check both depart_at and arrive_by attributes.

Use only high reachable edges for waypoints in case of fail

6d35ea4

genadz marked this pull request as draft March 4, 2021 21:27

genadz added 2 commits March 5, 2021 00:31

Update changelog

93047fb

Merge branch 'master' into kgv_reachable_waypoints

da921a1

kevinkreiser reviewed Mar 5, 2021

View reviewed changes

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

genadz and others added 4 commits March 5, 2021 15:07

Remove additional high_reachability flag from PathEdge; use method in…

fa38583

…stead

Test routing via waypoints that are near edges with imbalanced reacha…

409f89b

…bility.

Fix comment.

9604f6b

Fix unittest: use 'break_through' type for waypoints

984c2b9

genadz marked this pull request as ready for review March 5, 2021 14:24

genadz requested review from kevinkreiser, danpat, merkispavel and SvetlanaBayarovich March 5, 2021 14:30

danpat mentioned this pull request Mar 5, 2021

Waypoints near edges with poor reachability leads to no route #2872

Closed

4 tasks

Probably fix compilation on windows

c05038c

Merge branch 'master' into kgv_reachable_waypoints

2b7d88a

merkispavel reviewed Mar 10, 2021

View reviewed changes

Add multipoint routes to test requests

5f9702b

* there are 2 types of routes: 1) with middle points on oneway deadends; 2) points sutiated in different small connectivity components. We should provide some paths instead of "NoRoute" errors that we have right now.

genadz commented Mar 10, 2021

View reviewed changes

genadz added 3 commits March 10, 2021 20:45

Merge branch 'master' into kgv_reachable_waypoints

19bac23

Merge branch 'master' into kgv_reachable_waypoints

9bbf322

Merge branch 'master' into kgv_reachable_waypoints

2831573