second pass for CostMatrix #4536

nilsnolde · 2024-01-29T14:58:28Z

based on #4535

EDIT: on second thought, I'll leave TDMatrix as TODO for now. Less to review and less relevant overall. Not even sure it makes sense at all. Both TDMatrix algos don't seem to care about nothru pruning or even destonly (yet), hierarchies are not relevant either. Only thing they care about are conditional destonly but those are super rare.

TODO:

costmatrix
~~[ ] timedistancematrix~~
~~[ ] timedistancebssmatrix~~
tests
add a config parameter to control this?
Add a warning how many connections needed a 2nd pass

thoughts:

need to go through costmatrix again and make sure when a connection A -> B is not found, but B -> A is, that both A & B are doing another half-dijkstra, otherwise we don't get the bidirectional benefit; but of course need to additionally make sure they're not looking for anything else.. EDIT: checking the code again and running manually through the tests in debugging mode (impossible to prove with a test), I can confirm this is what's happening.

…also unify its signature; preparation for second pass on matrix

…xalgorithm.h

…pler albeit likely less peformant.. added tests to prove that it works, e.g. if A -> B is found but not B -> A, that only B -> A is re-computed, both with no-thru & destonly restrictions

nilsnolde · 2024-01-30T23:46:42Z

src/thor/bidirectional_astar.cc

@@ -90,7 +90,7 @@ void BidirectionalAStar::Clear() {
  // Set the ferry flag to false
  has_ferry_ = false;
  // Set not thru pruning to true
-  not_thru_pruning_ = true;
+  set_not_thru_pruning(true);


there's a function for this, I find it nicer for comprehension to use it

nilsnolde · 2024-01-30T23:48:11Z

src/thor/costmatrix.cc

+    if (costing_->pass() > 0 && !(matrix.second_pass(connection_idx))) {
+      continue;
+    }


if we're on the second pass and we didn't set the flag on the first pass, then don't bother processing anymore

nilsnolde · 2024-01-30T23:48:55Z

src/thor/costmatrix.cc

-      auto* pbf_time_zone_name = matrix.mutable_time_zone_names()->Add();
-      *pbf_time_zone_name = "";
+      // let's try a second pass for this connection
+      matrix.mutable_second_pass()->Set(connection_idx, true);


second_pass is defaulting to false, true is signaling the second pass that it wasn't found initially

nilsnolde · 2024-01-30T23:49:51Z

src/thor/costmatrix.cc

+        best_connection_.emplace_back(empty, empty, Cost{0.0f, matrix.times(connection_idx)},
+                                      matrix.distances(connection_idx));
+        best_connection_.back().found = true;


if we have previously found connections, we transfer the important attributes which make up the response

nilsnolde · 2024-01-30T23:50:33Z

src/thor/costmatrix.cc

    if (!s.unfound_connections.empty()) {
      locs_remaining_[MATRIX_FORW]++;
+    } else {
+      // don't look at sources which don't have unfound connections, important for second pass
+      s.threshold = 0;


setting the threshold to 0 for sources which have all destination found on the first pass, makes sure we don't even expand those sources anymore

nilsnolde · 2024-01-30T23:51:26Z

src/thor/costmatrix.cc

@@ -538,7 +550,7 @@ bool CostMatrix::ExpandInner(baldr::GraphReader& graphreader,
                        pred_dist, newcost.cost);
  }

-  return true;
+  return !(pred.not_thru_pruning() && meta.edge->not_thru());


allow u-turns if we hit a not thru edge

nilsnolde · 2024-01-30T23:53:21Z

src/thor/route_action.cc

@@ -312,6 +312,7 @@ std::vector<std::vector<thor::PathInfo>> thor_worker_t::get_path(PathAlgorithm*

  // Check if we should run a second pass pedestrian route with different A*
  // (to look for better routes where a ferry is taken)
+  // TODO(nils): how would a second pass find a better route, if it changes nothing ferry-related?


this seemed strange to me

nilsnolde · 2024-01-30T23:54:42Z

src/thor/matrix_action.cc

+  // TODO(nils): TDMatrix doesn't care about either destonly or no_thru
+  if (algo->name() != "costmatrix") {
+    algo->SourceToTarget(request, *reader, mode_costing, mode,
+                         max_matrix_distance.find(costing)->second);
+    return tyr::serializeMatrix(request);
+  }


we don't do second passes for tdmatrices (bss or normal), both don't have a concept of neither destonly nor not_thru

actually unidir a* also allows for destonly, not sure why?

nilsnolde · 2024-01-30T23:58:43Z

src/thor/costmatrix.cc

+      *matrix.mutable_date_times(connection_idx) = dt_info.date_time;
+      *matrix.mutable_time_zone_offsets(connection_idx) = dt_info.time_zone_offset;
+      *matrix.mutable_time_zone_names(connection_idx) = dt_info.time_zone_name;


I also changed this whole logic, it became unbearable now that we're "randomly" processing connections, i.e. not in sequence anymore. I realized that RepeatedPtrFields don't have a Set() method but also their operator() methods aren't const, so we can just assign new values. probably that's quite a bit worse for performance, but I doubt it's very measurable wrt to the whole expansion.

basically now we're initializing all repeated string fields with "" in reserve_pbf_arrays(), just like the other fields which have Set()

valhalla/thor/costmatrix.h

…ning if we had to do a second pass

valhalla/thor/matrixalgorithm.h

test/test.cc

valhalla/thor/costmatrix.h

kevinkreiser · 2024-01-31T14:29:02Z

this looks broadly good to me other than the tiny nitpicks we just discussed. let me know when those are patched and ill put a ship. would also be nice if @dnesbitt61 had a sec to look

nilsnolde · 2024-01-31T14:55:50Z

@kevinkreiser I addressed the comments

nilsnolde added 21 commits January 28, 2024 22:15

add a matrix parent class with SourcesToTargets as virtual function, …

96cb6d6

…also unify its signature; preparation for second pass on matrix

Merge branch 'master' into nn-matrix-refactor

da69b08

matrix test warning-free

9f3f340

less includes

6919d7c

Update route_action.cc

96deff2

remove matrix_common.h

23aa7fb

there was still a expansion_callback in costmatrix.h

656352d

why tz was updated?..

5f93bc5

changelog

e4263ce

remove has_time_ field from costmatrix which is duplicated with matri…

fefe4d9

…xalgorithm.h

also make Clear() virtual

952bc17

handle not_thru_pruning in base class

f3ad96f

most logic is done for costmatrix; needs some testing

6d3e753

fix valhalla_run_matrix

c35d876

another clear() missed in bench

d90c569

Merge branch 'master' into nn-matrix-refactor

c5f45f5

Merge branch 'master' into nn-matrix-second-pass

f250456

some fixed for CostMatrix

1e168ab

Merge branch 'nn-matrix-refactor' into nn-matrix-second-pass

0373ba0

changelog

395b519

more fixes: refactor the date_time handling in matrix and make it sim…

8cdd65e

…pler albeit likely less peformant.. added tests to prove that it works, e.g. if A -> B is found but not B -> A, that only B -> A is re-computed, both with no-thru & destonly restrictions

nilsnolde force-pushed the nn-matrix-second-pass branch from b14fe61 to 8cdd65e Compare January 30, 2024 20:18

nilsnolde changed the title ~~second pass for matrix~~ second pass for CostMatrix Jan 30, 2024

nilsnolde added 2 commits January 30, 2024 22:33

refactor a bit

bc85c61

comment

a45b1cd

nilsnolde commented Jan 30, 2024

View reviewed changes

nilsnolde commented Jan 31, 2024

View reviewed changes

valhalla/thor/costmatrix.h Outdated Show resolved Hide resolved

nilsnolde requested review from dnesbitt61 and kevinkreiser January 31, 2024 00:03

nilsnolde added 5 commits January 31, 2024 11:50

Merge branch 'master' into nn-matrix-second-pass

ef4d2f4

add config parameter to control turning the new feature on; add a war…

d46e5a9

…ning if we had to do a second pass

make the algo's name static

f9e6324

tidy

bc718c1

Merge branch 'master' into nn-matrix-second-pass

2496fd6

nilsnolde commented Jan 31, 2024

View reviewed changes

valhalla/thor/matrixalgorithm.h Show resolved Hide resolved

nilsnolde commented Jan 31, 2024

View reviewed changes

test/test.cc Outdated Show resolved Hide resolved

nilsnolde commented Jan 31, 2024

View reviewed changes

valhalla/thor/costmatrix.h Outdated Show resolved Hide resolved

address review comments

6796364

more tidy

65a7337

kevinkreiser approved these changes Jan 31, 2024

View reviewed changes

nilsnolde merged commit 3a92152 into master Feb 2, 2024
9 checks passed

nilsnolde deleted the nn-matrix-second-pass branch February 2, 2024 18:10

radekvermirovsky mentioned this pull request Feb 6, 2024

costmatrix second pass configuration parameter name inconsistency in valhalla_build_config #4565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

second pass for CostMatrix #4536

second pass for CostMatrix #4536

nilsnolde commented Jan 29, 2024 •

edited

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 30, 2024

nilsnolde Jan 31, 2024 •

edited

kevinkreiser commented Jan 31, 2024

nilsnolde commented Jan 31, 2024

second pass for CostMatrix #4536

second pass for CostMatrix #4536

Conversation

nilsnolde commented Jan 29, 2024 • edited

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

nilsnolde Jan 31, 2024 • edited

Choose a reason for hiding this comment

kevinkreiser commented Jan 31, 2024

nilsnolde commented Jan 31, 2024

nilsnolde commented Jan 29, 2024 •

edited

nilsnolde Jan 31, 2024 •

edited