add traffic to CostMatrix #4071

nilsnolde · 2023-04-13T18:03:19Z

Extending the work done in #3795, I added time awareness to the CostMatrix algo as well.

A few high-level things to mention:

currently we only support adding depart_at/current time spec, i.e. only on origin locations. I left a TODO for arrive_by which could be done (also for bidir A*), but not a priority for me at least and also quite a bit more work/complexity
the whole decision model in thor/matrix_action changed a bit. It still preserves the previous way we decided on the matrix algo, so nothing will change for Valhalla deployments which don't have any time info
unless prioritize_bidirectional: true is set (and of course plus a valid time on the sources/targets), we'll favor the more accurate (albeit much slower) TimeDistance matrix algo
to trigger a CostMatrix with time awareness:
- add "prioritize_bidirectional": true in the root of the request
- add at least one date_time string to an origin location or set the global date_time.type = 1/0

…nce traffic implementation anymore; needs a different server configuration which only allows timedistance

…-traffic

nilsnolde · 2023-04-13T18:04:28Z

I'll leave it here for now. Next week I'll be on vacation, I'll comment more after. Feel free to have a glance of course:)

src/sif/recost.cc

nilsnolde · 2023-04-15T13:30:34Z

src/thor/bidirectional_astar.cc

+    // TODO: actually we should not ignore access restrictions: if the reverse path
+    //   traversed a closed edge due to time restrictions, we could do a mini traversal
+    //   to circumvent the closed edge(s)


it's not the nicest patch, but still nicer than navigating over an edge we know is closed

nilsnolde · 2023-04-15T13:31:37Z

src/thor/costmatrix.cc

-    const float max_matrix_distance) {
+    const sif::travel_mode_t mode,
+    const float max_matrix_distance,
+    const bool has_time,


we tell it when there's a valid time. we compute it in matrix_action where we already check if any time is valid

nilsnolde · 2023-04-15T13:50:26Z

src/thor/costmatrix.cc

+    };
+
+    Cost new_cost{0.f, 0.f};
+    const auto label_cb = [&new_cost](const EdgeLabel& label) { new_cost = label.cost(); };


we only need to update the cost when recosting the path, no path necessary. once we want to tackle #3920, we can do it similar to bidir a*

nilsnolde · 2023-04-15T13:50:54Z

src/thor/costmatrix.cc

+    const auto edge_cb = [&edge_itr, &path_edges]() {
+      return (edge_itr == path_edges.end()) ? GraphId{} : (*edge_itr++);
+    };


same as bidir a*, only the label callback changes

nilsnolde · 2023-04-15T14:00:44Z

src/thor/matrix_action.cc

-
-// return true if any location had a time set
-// also disable time if it doesn't make sense computationally
-bool has_time(Api& request) {


changed it to check_matrix_time and moved it to matrix_common.h

nilsnolde · 2023-04-15T14:02:17Z

src/thor/matrix_action.cc

+  bool has_time =
+      check_matrix_time(request,
+                        options.prioritize_bidirectional() ? MatrixType::Cost : MatrixType::TimeDist);
+  if (has_time && !options.prioritize_bidirectional() && !force_costmatrix) {


do a timedistance matrix, no matter the costing model, if:

there's time

prioritize_bidirectional is false

the server doesn't have COST_MATRIX defined

document & warn properly

nilsnolde · 2023-04-15T14:03:04Z

src/thor/matrix_action.cc

+                        options.prioritize_bidirectional() ? MatrixType::Cost : MatrixType::TimeDist);
+  if (has_time && !options.prioritize_bidirectional() && !force_costmatrix) {
+    return tyr::serializeMatrix(request, timedistancematrix(), distance_scale, MatrixType::TimeDist);
+  } else if (has_time && options.prioritize_bidirectional()) {


otherwise, if there's a valid time AND prioritize_bidirectional is true, we do a CostMatrix

nilsnolde · 2023-04-15T14:03:20Z

src/thor/matrix_action.cc

+  } else if (matrix_type == MatrixType::Cost) {
+    return tyr::serializeMatrix(request, costmatrix(has_time), distance_scale, matrix_type);
+  } else {
+    return tyr::serializeMatrix(request, timedistancematrix(), distance_scale, matrix_type);


these two are the same as before

nilsnolde · 2023-04-15T14:04:03Z

src/thor/optimized_route_action.cc

+                                check_matrix_time(request, MatrixType::Cost),
+                                options.date_time_type() == Options::invariant);


also enable time awareness for /optimized

nilsnolde · 2023-04-15T14:04:47Z

src/thor/timedistancematrix.cc

@@ -22,37 +22,6 @@ static bool IsTrivial(const uint64_t& edgeid,
  }
  return false;
 }
-
-// Will return a destination's date_time string
-std::string GetDateTime(const std::string& origin_dt,


moved this to matrix_common.h as well so we can use it in both

nilsnolde · 2023-04-15T14:05:46Z

src/tyr/matrix_serializer.cc

@@ -2,7 +2,7 @@

 #include "baldr/json.h"
 #include "proto_conversions.h"
-#include "thor/costmatrix.h"
+#include "thor/matrix_common.h"


I moved most common things, which were actually previously defined in costmatrix.h to matrix_common.h as the import pattern was a bit weird otherwise (timedistance.h matrix would import costmatrix.h..)

nilsnolde · 2023-04-15T14:06:55Z

src/tyr/matrix_serializer.cc

+  json->emplace("algorithm",
+                std::string(matrix_type == MatrixType::Cost ? "costmatrix" : "timedistancematrix"));


add a algorithm field to the response to both osrm and valhalla formats

nilsnolde · 2023-04-15T14:19:18Z

valhalla/thor/costmatrix.h

-constexpr float kMaxCost = 99999999.9999f;
-
-// Time and Distance structure
-struct TimeDistance {


moved these to matrix_common.h as well

nilsnolde · 2023-04-15T14:19:51Z

valhalla/thor/matrix_common.h

@@ -15,15 +15,18 @@
 #include <valhalla/sif/dynamiccost.h>
 #include <valhalla/sif/edgelabel.h>
 #include <valhalla/thor/astarheuristic.h>
-#include <valhalla/thor/costmatrix.h>


this was strange 😄

…shortcuts in the algorithm, we'd need to hack the hierarchy limits

src/thor/matrix_action.cc

src/thor/costmatrix.cc

valhalla/thor/costmatrix.h

…er requested

nilsnolde · 2023-04-28T12:00:32Z

Addressed the review comments from yesterday @kevinkreiser :

9419e4b: re-arranged the decision model for which algo to use in matrix_action a little
ada29d7: Also added warnings if the config forces the algorithm but the request im-/explicitly suggested a different algorithm.
49ff04b: minor things like leaving a todo & changing < to <= in one place

nilsnolde · 2023-04-28T12:50:54Z

Anecdotally, one client was benchmarking the time results with the bidir A*. They computed a route for every 5 mins of the week (only with historical traffic) and it’s aligning perfectly, BUT the matrix is in ~60% of the cases off by one second (matrix reported exactly 1 sec longer than routing). Not sure if that’s the serializers or in the algos (though doesn’t look/feel like it’s the algos..). Not a show stopper IMO, but still worth noting.

nilsnolde · 2023-05-04T21:48:05Z

There was a minor conflict in costmatrix.cc. After CI is done, this is good to merge from my side @kevinkreiser :)

kevinkreiser · 2023-05-05T10:52:18Z

looks like setsources is missing the timeinfo arg

nilsnolde · 2023-05-05T19:19:34Z

Yeah there were a few code merge conflicts and I shouldn't have tried solving those in the Github UI, it made a bit of a mess. That function was called twice, old & new way, so it threw.

I fixed it.

kevinkreiser · 2023-05-09T10:52:15Z

src/thor/costmatrix.cc

+  source_edgelabel_.resize(source_count_);
+  source_edgestatus_.resize(source_count_);


arent these now redundant with Initialize?

…-traffic

nilsnolde · 2023-05-09T12:47:21Z

no coverage report, due to the github outage I assume, but we already know it's slightly failing due to not being able to configure hierarchy limits in the config. win succeeded, even though showing as "Queued"

Related work items: valhalla#4062, valhalla#4071, valhalla#4080, valhalla#4084, valhalla#4087, valhalla#4093, valhalla#4154, valhalla#4159, valhalla#4170, valhalla#4175, valhalla#4179, valhalla#4182, valhalla#4189, valhalla#4204, valhalla#4207, valhalla#4209, valhalla#4227, valhalla#4231, valhalla#4233

nilsnolde added 8 commits March 27, 2023 02:11

generally works; needs more verification, we don't test the timedista…

258f3c7

…nce traffic implementation anymore; needs a different server configuration which only allows timedistance

implement traffic into costmatrix

cd80ec4

test costmatrix traffic

7021456

changelog

85f9b38

Merge remote-tracking branch 'upstream/master' into nn-costmatrix-add…

ddeaadb

…-traffic

another accidental rapidjson update

2bd42d8

minor fixes

3a52971

changelog

b64618a

nilsnolde requested review from dnesbitt61 and kevinkreiser April 13, 2023 18:04

nilsnolde changed the title ~~Nn costmatrix add traffic~~ add traffic to CostMatrix Apr 13, 2023

nilsnolde added 2 commits April 15, 2023 14:56

Merge branch 'master' into nn-costmatrix-add-traffic

8747379

messed up when refactoring

2cf633f

nilsnolde commented Apr 15, 2023

View reviewed changes

src/sif/recost.cc Outdated Show resolved Hide resolved

nilsnolde commented Apr 15, 2023

View reviewed changes

apply own review

4f97382

nilsnolde added 2 commits April 21, 2023 06:45

make it a float

78787cd

add shortcuts to the map, but it'd need quite a bit more work to see …

62ab13a

…shortcuts in the algorithm, we'd need to hack the hierarchy limits

nilsnolde commented Apr 27, 2023

View reviewed changes

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

kevinkreiser reviewed Apr 27, 2023

View reviewed changes

src/thor/costmatrix.cc Show resolved Hide resolved

nilsnolde commented Apr 27, 2023

View reviewed changes

valhalla/thor/costmatrix.h Show resolved Hide resolved

nilsnolde added 3 commits April 28, 2023 13:42

re-arrange a bit the matrix_action decision model

9419e4b

add warnings when the config sets a different matrix algo than the us…

ada29d7

…er requested

minor things: leave a todo, <= instead of < to preserve old behavior

49ff04b

nilsnolde added 2 commits April 28, 2023 14:01

Merge branch 'master' into nn-costmatrix-add-traffic

283a005

changelog

72b6719

nilsnolde requested a review from kevinkreiser April 28, 2023 12:02

Merge branch 'master' into nn-costmatrix-add-traffic

af6846a

fix merge conflicts

5d7724a

Merge branch 'master' into nn-costmatrix-add-traffic

9940e66

kevinkreiser reviewed May 9, 2023

View reviewed changes

nilsnolde added 2 commits May 9, 2023 13:09

addressed review

80bd430

Merge remote-tracking branch 'upstream/master' into nn-costmatrix-add…

d2899b9

…-traffic

kevinkreiser approved these changes May 9, 2023

View reviewed changes

nilsnolde merged commit a2481fb into master May 9, 2023
1 of 2 checks passed

nilsnolde deleted the nn-costmatrix-add-traffic branch May 9, 2023 12:47

nilsnolde mentioned this pull request Aug 3, 2023

remove mapbox valhalla gis-ops/routingpy#114

Merged

nilsnolde mentioned this pull request Oct 28, 2023

refactor datetime output #4365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add traffic to CostMatrix #4071

add traffic to CostMatrix #4071

nilsnolde commented Apr 13, 2023 •

edited

Loading

nilsnolde commented Apr 13, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023 •

edited

Loading

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 27, 2023 •

edited

Loading

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023 •

edited

Loading

nilsnolde Apr 15, 2023

nilsnolde Apr 15, 2023

nilsnolde commented Apr 28, 2023

nilsnolde commented Apr 28, 2023

nilsnolde commented May 4, 2023

kevinkreiser commented May 5, 2023

nilsnolde commented May 5, 2023

kevinkreiser May 9, 2023

nilsnolde commented May 9, 2023

		check_matrix_time(request, MatrixType::Cost),
		options.date_time_type() == Options::invariant);

		json->emplace("algorithm",
		std::string(matrix_type == MatrixType::Cost ? "costmatrix" : "timedistancematrix"));

		source_edgelabel_.resize(source_count_);
		source_edgestatus_.resize(source_count_);

add traffic to CostMatrix #4071

add traffic to CostMatrix #4071

Conversation

nilsnolde commented Apr 13, 2023 • edited Loading

nilsnolde commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde Apr 15, 2023 • 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

nilsnolde Apr 27, 2023 • 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

nilsnolde Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde commented Apr 28, 2023

nilsnolde commented Apr 28, 2023

nilsnolde commented May 4, 2023

kevinkreiser commented May 5, 2023

nilsnolde commented May 5, 2023

Choose a reason for hiding this comment

nilsnolde commented May 9, 2023

nilsnolde commented Apr 13, 2023 •

edited

Loading

nilsnolde Apr 15, 2023 •

edited

Loading

nilsnolde Apr 27, 2023 •

edited

Loading

nilsnolde Apr 15, 2023 •

edited

Loading