street_side_tolerance and search_cutoff configurable and request parameters #1777

kevinkreiser · 2019-04-24T21:20:36Z

this make street_side_tolerance and search_cutoff configurable via the json conf but also as individual location attributes on the request. previously they were constants at the top of loki::search.

… time parameters

kevinkreiser · 2019-04-24T21:21:55Z

proto/tripcommon.proto

  optional uint64 way_id = 16;
-  optional uint32 minimum_reachability = 17 [default = 0];
-  optional uint32 radius = 18 [default = 0];


we no longer let the pbf default these because that nukes the ability to default them via config

kevinkreiser · 2019-04-24T21:22:25Z

src/baldr/location.cc

 Location::Location(const midgard::PointLL& latlng,
                   const StopType& stoptype,
                   unsigned int minimum_reachability,
                   unsigned long radius,
                   const PreferredSide& side)
    : latlng_(latlng), stoptype_(stoptype), minimum_reachability_(minimum_reachability),
-      radius_(radius), preferred_side_(side) {
+      radius_(radius), preferred_side_(side), node_snap_tolerance_(5), heading_tolerance_(60),
+      search_cutoff_(35000), street_side_tolerance_(5) {


default them on the custom class

kevinkreiser · 2019-04-24T21:22:41Z

src/loki/search.cc

-constexpr float SIDE_OF_STREET_SNAP = 25.f;
-
-// cone width to use for cosine similarity comparisons for favoring heading
-constexpr float DEFAULT_ANGLE_WIDTH = 60.f;


these are all request time parameters and their defaults are handled via config

kevinkreiser · 2019-04-24T21:23:14Z

src/loki/search.cc

@@ -387,7 +374,8 @@ struct bin_handler_t {
        length_ratio = 1.f - length_ratio;
      }
      // side of street
-      auto side = candidate.get_side(location.latlng_, candidate.sq_distance);
+      auto sq_tolerance = location.street_side_tolerance_ * location.street_side_tolerance_;
+      auto side = candidate.get_side(location.latlng_, candidate.sq_distance, sq_tolerance);


seemed odd to have the request parameter and the config be sq tolerance so we just square them as we go

kevinkreiser · 2019-04-24T21:23:35Z

src/loki/worker.cc

+        location.set_search_cutoff(default_search_cutoff);
+
+      if (!location.has_street_side_tolerance())
+        location.set_street_side_tolerance(default_street_side_tolerance);


this is where we set the defaults from config if the request didnt have them

kevinkreiser · 2019-04-24T21:23:45Z

src/loki/worker.cc

+  default_node_snap_tolerance = config.get<unsigned int>("loki.service_defaults.node_snap_tolerance");
+  default_search_cutoff = config.get<unsigned int>("loki.service_defaults.search_cutoff");
+  default_street_side_tolerance =
+      config.get<unsigned int>("loki.service_defaults.street_side_tolerance");


where we get the defaults from config

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

src/worker.cc

+            rapidjson::get_optional<unsigned int>(r_loc, "/street_side_tolerance");
+        if (street_side_tolerance) {
+          location->set_street_side_tolerance(*street_side_tolerance);
+        }


if they are supplied on the request we parse them to pbf here

kevinkreiser · 2019-04-24T21:24:43Z

test/actor.cc

@@ -30,7 +30,7 @@ boost::property_tree::ptree make_conf() {
      "loki":{
        "actions":["locate","route","sources_to_targets","optimized_route","isochrone","trace_route","trace_attributes","transit_available"],
        "logging":{"long_request": 100},
-        "service_defaults":{"minimum_reachability": 50,"radius": 0}
+        "service_defaults":{"minimum_reachability": 50,"radius": 0,"search_cutoff": 35000, "node_snap_tolerance": 5, "street_side_tolerance": 5, "heading_tolerance": 60}


lots of updates like this. they are required config items so tests fail without them

kevinkreiser · 2019-04-24T21:25:38Z

valhalla/baldr/pathlocation.h

+    }
+    if (loc.has_street_side_tolerance()) {
+      l.street_side_tolerance_ = loc.street_side_tolerance();
+    }


this is the important bit when moving from pbf to custom classes. tests coming shortly

…ard limit

kevinkreiser · 2019-04-30T13:14:10Z

docs/api/turn-by-turn/api-reference.md

@@ -39,6 +39,9 @@ To build a route, you need to specify two `break` locations. In addition, you ca
 | `radius` | The number of meters about this input location within which edges (roads between intersections) will be considered as candidates for said location. When correlating this location to the route network, try to only return results within this distance (meters) from this location. If there are no candidates within this distance it will return the closest candidate within reason. If this value is larger than the configured service limit it will be clamped to that limit. The default is 0 meters. |
 | `rank_candidates` | Whether or not to rank the edge candidates for this location. The ranking is used as a penalty within the routing algorithm so that some edges will be penalized more heavily than others. If `true` candidates will be ranked according to their distance from the input and various other attributes. If `false` the candidates will all be treated as equal which should lead to routes that are just the most optimal path with emphasis about which edges were selected. |
 | `preferred_side` | If the location is not offset from the road centerline or is closest to an intersection this option has no effect. Otherwise the determined side of street is used to determine whether or not the location should be visited from the `same`, `opposite` or `either` side of the road with respect to the side of the road the given locale drives on. In Germany (driving on the right side of the road), passing a value of `same` will only allow you to leave from or arrive at a location such that the location will be on your right. In Australia (driving on the left side of the road), passing a value of `same` will force the location to be on your left. A value of `opposite` will enforce arriving/departing from a location on the opposite side of the road from that which you would be driving on while a value of `either` will make no attempt limit the side of street that is available for the route. |
+| `search_cutoff` | The cutoff at which we will assume the input is too far away from civilisation to be worth correlating to the nearest graph elements |
+| `node_snap_tolerance` | During edge correlation this is the tolerance used to determine whether or not to snap to the intersection rather than along the street, if the snap location is within this distance from the intersection the intersection is used instead |


this one has been in here for a while but just wasnt documented

Can you add a note about the type of value expected for node_snap_tolerance and the default value? I believe it's a distance in meters defaulting to 5m?

kevinkreiser · 2019-04-30T13:19:51Z

src/loki/search.cc

@@ -340,7 +338,7 @@ struct bin_handler_t {
          PathLocation::PathEdge path_edge{std::move(other_id),
                                           1.f,
                                           node_ll,
-                                           score,
+                                           distance,


so the member of PathEdge used to be called score and the idea was that we would put more than just distance into the score. Over time, the fact that score was just distance became exploited down stream (meili and thor). Its clear that we still want to do a score, for proper penalization in the future however its also clear there is a need for distance. for now we'll call everything distance because that is what it is. and in the future we can add back a new field called score

kevinkreiser · 2019-04-30T13:21:49Z

test/filesystem.cc

-  for (const auto& d : dirs)
-    filesystem::remove(d);
+  for (auto d = dirs.rbegin(); d != dirs.rend(); ++d)
+    filesystem::remove(*d);


this test failed on every other run. you have to delete the directories from the leaves up to the root otherwise it fails.

…t builds wont fail because of noise in the node_lls loaded from the tiles

kevinkreiser · 2019-05-01T01:17:16Z

src/loki/search.cc

@@ -299,7 +290,7 @@ struct bin_handler_t {
                      PathLocation& correlated,
                      std::vector<PathLocation::PathEdge>& filtered) {
    // we need this because we might need to go to different levels
-    auto score = candidate.point.Distance(location.latlng_);


this was wrong! what this says is store the distance from the input location to the snapped location along the edge. the thing, this is the function that correlates a result to a node, ie a node_snap. which means we really should be measuring the distance from the input location to the node_ll.

ive changed it to do so, which made the 32bit build fail. it failed because the precision of the node_ll that it gets out of the graph tile made it such that the node_snap measured about 9cm away from the input location, instead of 0 which it should have been.

kevinkreiser · 2019-05-01T01:22:51Z

src/baldr/pathlocation.cc

@@ -46,7 +46,7 @@ bool PathLocation::shares_edges(const PathLocation& other) const {
    for (const auto& other_edge : other.edges) {
      if (edge.id == other_edge.id && edge.sos == other_edge.sos &&
          midgard::equal<float>(edge.percent_along, other_edge.percent_along) &&
-          midgard::similar<float>(edge.distance + 1, other_edge.distance + 1) &&
+          midgard::equal<float>(edge.distance, other_edge.distance, .1f) &&


change the distance equality check to be down to 10 centimeters so that floating point noise on 32bit platforms near the equator doesnt trip up distance checks. the 7th decimal place seems to be where this starts cropping up in ll distance computations which at the equator its close to 11 centimeters.

kevinkreiser added 2 commits April 24, 2019 17:18

make street_side_tolerance and search_cutoff configurable and request…

62edbe3

… time parameters

wrong pr

6754ae7

kevinkreiser commented Apr 24, 2019

View reviewed changes

kevinkreiser requested review from danpaz and gknisely April 24, 2019 21:25

kevinkreiser added 6 commits April 29, 2019 18:32

add tests for new parameters

9182aa5

extra protection against bad search cutoffs, cant be less than radius

616df5a

Merge branch 'master' into kk_cutoff

f66ecc5

fix filesystem test

6e5bea8

revert change in logic. radius is soft limit where search_cutoff is h…

c01e50e

…ard limit

Merge remote-tracking branch 'origin/kk_cutoff' into kk_cutoff

a7f3b5c

kevinkreiser commented Apr 30, 2019

View reviewed changes

small cleanups

b6e9af0

danpaz previously approved these changes Apr 30, 2019

View reviewed changes

loosen the distance part of the check in the search test so that 32bi…

9e2c41f

…t builds wont fail because of noise in the node_lls loaded from the tiles

kevinkreiser dismissed danpaz’s stale review via 9e2c41f May 1, 2019 00:41

kevinkreiser commented May 1, 2019

View reviewed changes

kevinkreiser requested a review from danpaz May 1, 2019 01:50

danpaz approved these changes May 1, 2019

View reviewed changes

kevinkreiser merged commit 00eaa93 into master May 1, 2019

kevinkreiser deleted the kk_cutoff branch June 6, 2019 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

street_side_tolerance and search_cutoff configurable and request parameters #1777

street_side_tolerance and search_cutoff configurable and request parameters #1777

kevinkreiser commented Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 24, 2019

kevinkreiser Apr 30, 2019

danpaz Apr 30, 2019

kevinkreiser Apr 30, 2019

kevinkreiser Apr 30, 2019

kevinkreiser May 1, 2019

kevinkreiser May 1, 2019

street_side_tolerance and search_cutoff configurable and request parameters #1777

street_side_tolerance and search_cutoff configurable and request parameters #1777

Conversation

kevinkreiser commented Apr 24, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment