Reachability... Round 2... FIGHT! #1965

kevinkreiser · 2019-10-09T22:53:12Z

The problem

Current reachability computations are broken. Let me count the ways...

The algorithm was very concerned with performance. Rightfully so, if you want to find a good edge you have to look at a lot of edges. Looking at a lot of edges and doing a graph expansion from every one is expensive. So the implementation did a graph coloring instead, essentially a flood fill, where we marked all nodes with the same color if they were on any path that connected them to each other. In the end this color was really just a count of the number of nodes that appear on this connected path.

I mean, it was an interesting idea: how many nodes are on any of the paths that leave this node? the problem with this is that it doesnt really answer the questions we want to answer. Namely:

for a given edge, how many nodes can i reach? (ie outbound reachability)
how many nodes can reach a given edge? (ie inbound reachability)

In the end, the coloring technique was basically a depth first forward expansion algorithm. Which means it did compute reachability but only in the forward direction. This was good for the origins of routes but it doesnt help for intermediate and final destinations because for those we need to actually know the inbound reachability (ie reverse expansion). Technically intermediate ones need to know both directions of reachability while origin locations only need to know outbound reachability.

You might be asking why do we care about directional reachability. The answer is there are degenerate cases in the graph where an edge is a oneway to a deadend or a oneway leaving a deadend. In both cases you have a situation where you either cant expand forward or you cant expand in reverse. This makes problems at destinations where we were checking outbound reachability but really we needed inbound reachability so that it is likely that thor will find a path into that edge.

The solution

So we needed to fix them by being less smart and more accurate. I've sketched out a more optimized and accurate algorithm but for the moment I've only implemented the naive and correct algorithm.

Instead of doing a reverse coloring I've implemented something more straight forward which is to return a struct denoting both the inbound and outbound reach of a given edge by doing a breadth first search in both the forward and reverse directions. The code is as a result much simpler to understand and at the same time no less performant. I ran RAD on the 13.8k routes in the auto.txt file and it took approximately 3.5% longer to complete on the new code, which averaged out over all the requests is about 0.94ms longer per request. Additionally, the number of route failures was decreased from 0.95% to 0.88% which makes me think that we could increase our default reachability threshold or these routes are just not having that many failiures in general. Ok increasing the default reach to 100 moved the failure rate to 0.82%.

…w different reachability for what should all be passable nodes and edges..

… one, use the right node when doing the reverse expansion. when routing we only need outbound reach at the origin and inbound reach at the destination

kevinkreiser · 2019-10-10T21:02:44Z

proto/tripcommon.proto

@@ -38,10 +38,12 @@ message Location {
    optional LatLng ll = 3;
    optional SideOfStreet side_of_street = 4;
    optional float distance = 5;
-    optional int32 minimum_reachability = 6;
+    optional int32 minimum_reachability = 6; //deprecated


now we have directional reachability so we no longer support a single non-sensical measure. note that this is on the output side. i should mention that i didnt update the input side meaning we still have only a single reachability as input used for both directions. this means for locate for example if we set reachability that both inbound and outbound have to meet the criteria, which is not that good for debugging.

kevinkreiser · 2019-10-10T21:03:25Z

src/baldr/graphtile.cc

-    const auto* edge = directededge(nodeinfo.edge_index());
-    return iterable_t<const DirectedEdge>{edge, nodeinfo.edge_count()};
+    const auto* nodeinfo = nodes_ + node.id();
+    return GetDirectedEdges(nodeinfo);


add a helper for this that takes a node instead of an id and call it from the one that takes an id

kevinkreiser · 2019-10-10T21:04:04Z

src/baldr/location.cc

-         minimum_reachability_ == o.minimum_reachability_ && radius_ == o.radius_ &&
-         preferred_side_ == o.preferred_side_;
+         min_outbound_reach_ == o.min_outbound_reach_ && min_inbound_reach_ == o.min_inbound_reach_ &&
+         radius_ == o.radius_ && preferred_side_ == o.preferred_side_;


lots of changes like this in here where we switch from single reach measure to directional reach measure

kevinkreiser · 2019-10-10T21:04:18Z

src/baldr/pathlocation.cc

@@ -26,7 +27,8 @@ PathLocation::PathLocation(const Location& location) : Location(location) {

 bool PathLocation::operator==(const PathLocation& other) const {
  // Check all of the scalar properties
-  if (other.minimum_reachability_ != minimum_reachability_ || other.radius_ != radius_ ||
+  if (other.min_outbound_reach_ != min_outbound_reach_ ||
+      other.min_inbound_reach_ != min_inbound_reach_ || other.radius_ != radius_ ||


again single measure to two measures

kevinkreiser · 2019-10-10T21:05:38Z

src/loki/reach.cc

+namespace valhalla {
+namespace loki {
+
+directed_reach SimpleReach(const DirectedEdge* edge,


this is the main funciton that computes the reach. ive commented the hell out of it so i wont write anything else here :)

kevinkreiser · 2019-10-10T21:06:51Z

src/loki/reach.cc

+}
+
+/*
+directed_reach Reach(const DirectedEdge* edge,


this is a sketch of a more performant algorithm where we try to "share" reach between edges on the same paths (similar to the flood fill or coloring approach we had before). this approach relies on a long lived cache that edges share with each other when their paths cross/merge. its just a sketch and we can delete it if we dont want/need to do it

kevinkreiser · 2019-10-10T21:07:35Z

src/loki/route_action.cc

@@ -88,7 +88,7 @@ void loki_worker_t::route(Api& request) {
  // correlate the various locations to the underlying graph
  std::unordered_map<size_t, size_t> color_counts;
  try {
-    auto locations = PathLocation::fromPBF(options.locations());
+    auto locations = PathLocation::fromPBF(options.locations(), true);


this little trick allows us to only care about outbound reach for origins and inbound reach for destinations. it lets us not cull otherwise good edges for these special case location candidates

kevinkreiser · 2019-10-10T21:10:00Z

src/loki/search.cc

-  };
+  // keep track of edges whose reachability we've already computed
+  // TODO: dont use pointers as keys, its safe for now but fancy caching one day could be bad
+  std::unordered_map<const DirectedEdge*, directed_reach> directed_reaches;


the main changes in this file rae to delete the previous implementation of reach and replace it with a call to the new one. i wanted to call out that i did this as a shortcut to prove this was working but i feel like we really shouldnt use pointers as keys

kevinkreiser · 2019-10-10T21:13:20Z

src/loki/search.cc

-                                  std::vector<projector_wrapper>::iterator end,
-                                  const GraphTile* tile,
-                                  const DirectedEdge* edge) {
+  directed_reach check_reachability(std::vector<projector_wrapper>::iterator begin,


here we actually decide if we should or shouldnt check the reach. there is an optimization in there where we say hey if we aren't going to actually get a better candidate by checking this candidates reach then dont waste our time, it seems in practice this really helps avoid any reach calculation at all. additionally even if we do get a better edge candidate we dont actually compute its reach if we already have reachable candidates.

kevinkreiser · 2019-10-10T21:15:41Z

src/loki/search.cc

+    // notice we do both directions here because in the end we use this reach for all input locations
+    auto reach =
+        SimpleReach(edge, max_reach_limit, reader, kInbound | kOutbound, edge_filter, node_filter);
+    directed_reaches[edge] = reach;


here we should do one more optimization. if the opposing edge is not filtered and its endnode (this edges begin node) is not filtered, then we can assign the same reachability to the opposing edge and not have to do the reach check.

kevinkreiser · 2019-10-10T21:17:46Z

src/loki/search.cc


      // keep the best point along this edge if it makes sense
      c_itr = bin_candidates.begin();
      for (p_itr = begin; p_itr != end; ++p_itr, ++c_itr) {
+        // TODO: update to use directional reach


crap i missed this

kevinkreiser · 2019-10-10T21:18:20Z

test/reach.cc

+    }
+  }
+
+  // throw std::runtime_error(std::to_string(edges));


crap another clean up item

kevinkreiser · 2019-10-10T21:19:09Z

test/reach.cc

+  return opp_edge->endnode();
+}
+
+void check_all_reach() {


this is a new test i added which scans over all edges in utrecht and checks that the reaches are not wrong. there could be more tests in here to be honest but the degenerate cases are covered the happy path cases are harder to write tests for

kevinkreiser · 2019-10-10T21:19:51Z

test/reach.cc

+
+      // if inbound is 0 and outbound is not then it must be an edge leaving a dead end
+      // meaning a begin node that is not accessable
+      // NOTE: but if the outbound is just 1 then we know this is an orphaned edge, meaning its a dead


note is not valid

kevinkreiser · 2019-10-10T21:20:43Z

valhalla/baldr/graphtile.h

-      const auto* trans = transition(nodeinfo.transition_index());
-      return midgard::iterable_t<const NodeTransition>{trans, nodeinfo.transition_count()};
+      const auto* nodeinfo = nodes_ + node.id();
+      return GetNodeTransitions(nodeinfo);


similar to the other helper i added so that we can use the node rather than the id

kevinkreiser · 2019-10-10T21:21:30Z

valhalla/baldr/pathlocation.h

    std::vector<Location> pls;
    for (const auto& l : locations) {
      pls.emplace_back(fromPBF(l));
    }
+    // for regular routing we dont really care about inbound reach for the origin or outbound reach
+    // for the destination so we remove that requirement
+    if (route_reach && pls.size() > 1) {


heres were we specialize for route locations

…each checks for a particular locations candidate

…he criteria we ought to mark it as such

kevinkreiser · 2019-10-15T19:00:53Z

src/loki/search.cc

+        // it's possible that it isnt reachable but the opposing is, switch to that if so
+        const GraphTile* opp_tile = tile;
+        const DirectedEdge* opp_edge = nullptr;
+        if (!reachable && (opp_edge = reader.GetOpposingEdge(edge, opp_tile)) &&
+            edge_filter(opp_edge) > 0.f) {
+          auto opp_reach = check_reachability(begin, end, opp_tile, opp_edge);
+          if (opp_reach.outbound >= p_itr->location.min_outbound_reach_ &&
+              opp_reach.inbound >= p_itr->location.min_inbound_reach_) {
+            tile = opp_tile;
+            edge = opp_edge;
+            reach = opp_reach;
+            reachable = true;
+          }
+        }


this is an important little bit of code. for regular routing, at the origin you need only outbound reach and at the destination you need only inbound reach (intermediate points need both). loki only checks one directed edge per normal edge in the graph. this meant that it wasnt checking reachability of the opposing edge. there are some cases were the best edge for an origin for example would deadend into a gate, but the opposing edge, going away from the gate would have fine reachability. this check allows us, in the case that the other edge didnt make the cut, to check if the opposing edge does.

you might notice that the check for reachable is per input location p_iter and that means this can flip each time. that is ok because the check_reachability function caches the result so it doesnt actually do more than 2 reach checks here.

kevinkreiser · 2019-10-15T19:02:26Z

src/loki/search.cc

+    const DirectedEdge* opp_edge = nullptr;
+    if (reach.outbound > 0 && reach.inbound > 0 && (opp_edge = reader.GetOpposingEdge(edge, tile)) &&
+        edge_filter(opp_edge) > 0.f)
+      directed_reaches[opp_edge] = reach;


this is an optimization which just says an opposing edge has the same reach if it can be reached by its opposing edge in both directions.

kevinkreiser added 5 commits October 7, 2019 18:27

outlines for thoughts on reach fixes

982ed3b

initial implementation, without check for correctness

8f44244

now with failing tests but geting close

8731e51

Merge remote-tracking branch 'origin/master' into reachability2

67acdc1

fix opposing edge from wrong tile and protect against partial data sets

74184c3

kevinkreiser requested review from dnesbitt61 and danpaz October 9, 2019 22:53

kevinkreiser added 4 commits October 10, 2019 00:54

stupid stupid lint

1b7ab0b

refactor loki to actually use new reach checks

a85fbb8

finally getting reach results through loki, reach checks in there sho…

4f38b3c

…w different reachability for what should all be passable nodes and edges..

shortcircuit if reach is 0, return max reach even if we find a higher…

8e18635

… one, use the right node when doing the reverse expansion. when routing we only need outbound reach at the origin and inbound reach at the destination

kevinkreiser commented Oct 10, 2019

View reviewed changes

test/reach.cc Outdated

}

}

// throw std::runtime_error(std::to_string(edges));

Copy link

Member Author

kevinkreiser Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crap another clean up item

kevinkreiser commented Oct 10, 2019

View reviewed changes

kevinkreiser requested a review from dgearhart October 10, 2019 21:23

kevinkreiser added 2 commits October 12, 2019 14:23

clean up some TODOs, add what is hopefully the last one

d8f750b

handle the case when we need to be looking at the opposing edge for r…

8e76cce

…each checks for a particular locations candidate

kevinkreiser added 3 commits October 15, 2019 10:39

if we are going to switch to the opposing edge because it does meet t…

08b761b

…he criteria we ought to mark it as such

Merge remote-tracking branch 'origin/master' into reachability2

6ceeada

update changelog

ea9cd21

kevinkreiser commented Oct 15, 2019

View reviewed changes

danpaz approved these changes Oct 15, 2019

View reviewed changes

danpaz merged commit 8e8e3de into master Oct 15, 2019

danpaz deleted the reachability2 branch October 15, 2019 21:55

kevinkreiser mentioned this pull request Oct 18, 2019

Reachability Doesnt Agree With Path Finding #1983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reachability... Round 2... FIGHT! #1965

Reachability... Round 2... FIGHT! #1965

kevinkreiser commented Oct 9, 2019 •

edited

Loading

kevinkreiser Oct 10, 2019 •

edited

Loading

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019 •

edited

Loading

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 10, 2019

kevinkreiser Oct 15, 2019

kevinkreiser Oct 15, 2019

Reachability... Round 2... FIGHT! #1965

Reachability... Round 2... FIGHT! #1965

Conversation

kevinkreiser commented Oct 9, 2019 • edited Loading

The problem

The solution

kevinkreiser Oct 10, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser Oct 10, 2019 • 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

kevinkreiser commented Oct 9, 2019 •

edited

Loading

kevinkreiser Oct 10, 2019 •

edited

Loading

kevinkreiser Oct 10, 2019 •

edited

Loading