-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introducing A Centroid/Converge/Rendezvous/Meet API #2734
Conversation
…ecific expansion. this allows us to use the same label set for multiple simultaneous path expansions
…uash this with the 2 previous commits
…and typos. it works!
@@ -276,7 +276,7 @@ static void BM_Sif_Allowed(benchmark::State& state) { | |||
// auto pred = sif::EdgeLabel(0, tgt_edge_id, edge, costs, 1.0, 1.0, | |||
// sif::TravelMode::kDrive,10,sif::Cost()); | |||
auto pred = sif::EdgeLabel(); | |||
int restriction_idx; | |||
uint8_t restriction_idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll see changes similar to this throughout, basically we didnt need 2^32 values to represent the restriction index for a restriction at a particular edge. if we ever find an edge that even has 256 restrictions i'll eat my hat 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious what the benefit is vs. the potential for overflow (though rare do you now have to do some bounds checking anywhere?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah inside the function that actually checks for restrictions (in dynamic cost) if the index is larger than 254 we cant tell what restriction was there in the route. note that this doesnt mean that we wont adhere to the restriction, it just means that the serializer doesnt know that a restriction was there.
* @param reader provides access to graph primitives | ||
* @return the constructed location | ||
*/ | ||
valhalla::Location make_centroid(const valhalla::baldr::GraphId& edge_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use an edge to make a destination location for the route
namespace thor { | ||
|
||
// constructor | ||
PathIntersection::PathIntersection(uint64_t edge_id, uint64_t opp_id, uint8_t location_count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class tracks potential convergence points. it uses masks to figure out which locations have found shortest paths to this edge. i had actually forgotten that i dont need to track both the edge id as well as its opposing. since we only use the smallest of the 2 edge ids to track that particular meeting point. so this is another TODO, we can remove the opp_id and save some ram. <-- Done!
|
||
// this is fired when the edge in the label has been settled (shortest path found) so we need to check | ||
// our intersections and add or update them | ||
thor::ExpansionRecommendation Centroid::ShouldExpand(baldr::GraphReader& reader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the meat of the algorithm. here we get informed that a specific location settled a specific edge, we check if we are already tracking it and we flip the bit corresponding to the input location that settled it. if we flipped the last outstanding bit in the mask, then its over and we found a least cost convergence point
src/thor/centroid.cc
Outdated
// walk edge labels to form paths for each location to the centroid | ||
template <typename label_container_t> | ||
std::vector<std::vector<PathInfo>> | ||
Centroid::FormPaths(const google::protobuf::RepeatedPtrField<valhalla::Location>& locations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing interesting here, just loop over the locations and recover their paths individually
src/thor/dijkstras.cc
Outdated
@@ -41,7 +41,8 @@ namespace thor { | |||
|
|||
// Default constructor | |||
Dijkstras::Dijkstras() | |||
: access_mode_(kAutoAccess), mode_(TravelMode::kDrive), adjacencylist_(nullptr) { | |||
: access_mode_(kAutoAccess), mode_(TravelMode::kDrive), adjacencylist_(nullptr), | |||
multipath_(false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main changes to dijkstras are a boolean to flag we want to do path multiplexing (this means assigning an id to each locations initial edges in the labelset) as well as actually passing those to the different functions on the labelset/edgestatus
@@ -205,6 +205,37 @@ std::string thor_worker_t::expansion(Api& request) { | |||
return rapidjson::to_string(dom, 5); | |||
} | |||
|
|||
void thor_worker_t::centroid(Api& request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the main place where new service/api related work was needed. here we do the same thing that a regular route request does but we call into the new algorithm and build a leg for each individual route that came out of it.
@@ -49,6 +49,16 @@ const std::unordered_map<std::string, float> kMaxDistances = { | |||
constexpr float kDistanceScale = 10.f; | |||
constexpr double kMilePerMeter = 0.000621371; | |||
|
|||
std::string serialize_to_pbf(Api& request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this into the anonymous namespace
src/thor/worker.cc
Outdated
@@ -142,7 +142,7 @@ thor_worker_t::work(const std::list<zmq::message_t>& job, | |||
} | |||
case Options::isochrone: | |||
result = to_response(isochrones(request), info, request); | |||
denominator = options.sources_size() * options.targets_size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noticed this bug and fixed it
src/tyr/route_serializer_valhalla.cc
Outdated
tyr::route_references(trip_json, api.trip().routes(0), api.options()); | ||
auto json = json::map({{"trip", trip_json}}); | ||
auto json = json::map({}); | ||
auto alternates = json::array({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where i added support for alternates in the valhalla route response format, pretty straight forward actually!
cost_(0, 0), sortcost_(0), distance_(0), transition_cost_(0, 0) { | ||
origin_(0), toll_(0), not_thru_(0), deadend_(0), on_complex_rest_(0), path_id_(0), | ||
restriction_idx_(0), cost_(0, 0), sortcost_(0), distance_(0), transition_cost_(0, 0) { | ||
assert(path_id_ <= baldr::kMaxMultiPathId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two main things i did here were add support for specifying a path id (optionally) and made restriction index manditory.
* @param set Label set for this directed edge. | ||
* @param index Index of the edge label. | ||
* @param tile Graph tile of the directed edge. | ||
* @param path_id Identifies which path the edge status belongs to when tracking multiple paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically all the functions here now have an optional path_id which gets bitwise or'd into the spare bits of the tile/level id so we can use the same edgestatus to track up to 127 paths
May want to add a maximum distance between locations as a protection against long running requests. One location in MD and one in CO took 90 seconds on my laptop. For now this seems to be a nice feature for short distances (which I would argue as a "meetup" most requests would likely be over short distances). |
Just trying it out myself, love the idea. A little feedback from a user's POV:
+1 for maximum distance config, took around 3-5 seconds (depending on options) for total 200 km of trips. Couldn't help but quickly implement it in the QGIS plugin to try it out;) (also returns the meeting point with total distance/duration) You can install from here if you wanna try: https://qgisrepo.gis-ops.com |
TODO:
|
…uilding into single function making config changes less of a hassle
Since I initially PRd this a lot has changed in the repo. I've resolved all the conflicts but I did do two large quality of life things in the tests:
I also added a minimal centroid unit test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
What is this?
This past weekend I was thinking about a problem. That problem was specifically:
My first thoughts on such an algorithm were essentially that classical algorithms won't work because:
My second thought was well what even is a "best" meeting place?
Once I had formulated the problem properly it was time to consider how the algorithm should work. For that I came up with a few requirements:
Problem 1: Efficient Path Multiplexing
The existing implementation of the priority queue (combo of edgelabel, edgestatus and double bucket queue) doesn't currently support tracking paths from multiple locations at once. In bidirectional a* and in cost matrix for example, where we need this kind of implementation, we instatiate two queues for each origin/destination pair. This has some drawbacks:
So I wanted to see if I could find a way to mark entries in the queue in such a way that I could tell which path expansion (location) they came from and differentiate them. Thus using a single queue for many path expansions at the same time (multiplexing).
To do this I was able to mark both the edge labels in the labelset and the edge statuses (index into the labelset) with a path id/index/color to differentiate which location a particular path was tracking. I was able to use the 7 spare bits in the tile id of the edge status and free up some bits in the edge label. Double bucket queue didn't need any changes because it works with labelset indices directly.
After those changes everything worked as normal without any changes to the other algorithms since the path id/index/color is optional. But what I want to try out is to switch to using just one queue in bidirectional a* to see if I can get a performance boost from not having to jump back and forth to 2 different memory locations. I'm considering PRing just this change separately but I'll get to that a bit later!
Problem 2: Core Algorithm
The algorithm itself is quite simple. Remember that we need to come up with a destination, which means we can't use any of the directed search algorithms (a* et al) that rely on knowing the destination. It would be nice if we could because they have better performance than Dijkstra's but the way they get that performance is precisely by using the goal heuristic to coach the path expansion toward the goal. In our case we cannot pick a goal that would generate an admissable heuristic which means thats off the table for us. But there are other tricks we can do to cull the search space. I'll get to those in the future work section.
So it's dijsktra's for us. No problem. Remember what the main objective is for convergence, we need to return the first edge in the graph to which all locations have found a shortest path. What that means is that, as we pop edges off the queue (ie. find shortest path from an origin to that edge) we need to track which other edges also found paths to this edge. To do that I created a small struct which holds 2 64bit masks. Each bit in the mask represents an origin that has either found or not found its shortest path to this edge. When I get the callback from dijsktras that we've settled an edge, I check which path/origin it was for and I flip that bit on my tracking struct. If that bit I just flipped was the last one to need flipping, then we have converged. Once that happens dijkstras stopps and we call
FormPaths
, which recoveres the edges of the path for each individual origin location (looks like an alternate routes result, ie has multiple routes in the output).Problem 3: HTTP API
More cool stuff to elaborate on here but the great news is that the input to the API already looks exactly like a normal
/route
request in that you need 2 or more locations. So I quickly added a new action to the request calledcentroid
and focused on the output. The good news there is that output looks exactly like a route withalternates=n
except nown
is the total number of input locations. Another thing I did here was modify the valhalla route serializer to support alternates (THANK GOD). The current implementation has something like{"trip":{... your route here ...}}
. This made it quite clunky to add alternates but I found a reasonable way:{"trip":{... your route here ...}, "alternates":[{... your alternate here ...}, ...]}
. I can PR this separately as well.Demo:
I cracked open vim and quickly hacked together a leaflet demo to show off this API. You simply click the locations on the map you want to use as your input locations and then press the button at the bottom to fire off the request. The green dots are origins and the red dot is the destination. PR in the demos repo is here: valhalla/demos#234
Future Work:
There are a number of things to do to make this work practical and useful. Some of which make sense to add to the API, some of which should be saved for other projects that make use of it. I'll list off a few:
Things that make sense to add:
x
meters of each other as they are too similar.Things that make sense for users of the API to add themselves:
Apart from that there are a lot of nice TODOs listed in the code which I'm not really worried about tackling in this iteration. I think its quite alright to offer this service as a fun little beta API for people to try out!
Unit Testing
This PR still needs unit tests, I'm working on those.