Refactor CostMatrix #4372

nilsnolde · 2023-10-31T02:33:30Z

Alright, now this should be good to go.. I'm (kinda) sorry to say this is a bit more than just a refactor. Since it touches the entirety of CostMatrix and we'd have to review it from the ground up, I decided to take the chance to fix a few more things:

the CostMatrix is now very close now to bidir A*, which should allow for some unification of the two to shed tons of code and make everything easier to maintain
being guided by the most advanced algorithm we have, CostMatrix is now a lot more robust, in particular towards deadends (where there were a lot of unfound connections before)
fixes Datetime in result elements of sources to targets are in UTC not local as documentation states #4360
implements More matrix clearing memory #4362 (already went over that with Dave)
similar to More matrix clearing memory #4362, it cleans up some things in the other algos

There's still quite a few things to do:

NOTE, I'm still owing a proper performance test with master, though following #4362 , I'm quite optimistic:)

…es:)

test/gurka/test_expansion.cc

src/thor/astar_bss.cc

src/thor/bidirectional_astar.cc

kevinkreiser · 2023-12-02T04:04:37Z

CHANGELOG.md

@@ -16,6 +16,7 @@
   * FIXED: recover proper shortest path to ferry connections (make sure correct label index is used) [#4378](https://github.com/valhalla/valhalla/pull/4378)
   * FIXED: Allow all roads for motorcycles [#4348](https://github.com/valhalla/valhalla/pull/4348)
   * FIXED: motorcar:conditional should not apply to motorcycle and moped [#4359](https://github.com/valhalla/valhalla/pull/4359)
+   * FIXED: lots of issues with CostMatrix (primarily deadend logic) with a complete refactor modeling things very close to bidir A*, also to prepare for a unification of the two [#4372](https://github.com/valhalla/valhalla/pull/4372)


nit: need to move this down one

Saw that too, wonder why there wasn’t a the usual conflict when master merged

kevinkreiser · 2023-12-02T04:08:36Z

run_route_scripts/run_with_server_matrix.py

@@ -0,0 +1,97 @@
+#!/usr/bin/env python3


do we actually need a separate script for this, the only thing that should really differ is the parsing of the response? the other script i think can let you run any file of requests so it shoudl work for the input part. and the output can be handled pretty easily by simply looking at the url to find which endpoint was hit route/sources_to_targets whatever and calling appropriate handler to parse the resulting json

i dont feel strongly about it though, but it feels to me like tech debt

tbh, the vast majority of python in this repo feels like tech debt (except for most installable scripts)😄 I should’ve done the work to clean this all up a bit more and not just copy paste. I’ll do that at some point I’m sure. If you want I can keep this locally instead of committing until I find the motivation to do that:)

nah its ok, i trust you to keep it on your list 😄

kevinkreiser · 2023-12-02T04:08:48Z

run_route_scripts/run_with_server_matrix.py

+    parser = argparse.ArgumentParser(description="Runs payloads generated by gen_requests_matrix.py against a Valhalla server and writes the results to a single file for diffing purposes. Also prints out the total time.")
+    parser.add_argument('--test-file', type=Path, help='The file with the test requests', required=True)
+    parser.add_argument('--url', type=str, help='The url to which you want to POST the request bodies',
+                        default='http://localhost:8002/route')


case in point haha

Na the other script is doing one file per request, for matrix that’s not cool

i was focused on this part

default='http://localhost:8002/**route**'

kevinkreiser · 2023-12-02T04:12:59Z

src/thor/astar_bss.cc

-    // Abort if max label count is exceeded
-    if (total_labels > max_label_count_) {
-      return {};
-    }



but dont we still want this? as i was saying there are two types of aborting, using the interrupt which is telling us the client said hey i dont care about this request anymore, and then the other kind where we say ok we searched hard enough that we now give up

I mentioned it somewhere: it’s not used at all. We still do what you’re saying, but it’s not done with this variable, it's this nc a bit further up (same for most other algos).

kevinkreiser · 2023-12-02T04:28:43Z

src/thor/bidirectional_astar.cc

-                                            {opp_edge, opp_edge_id,
-                                             edgestatus.GetPtr(opp_edge_id, tile)},
-                                            shortcuts, tile, offset_time);
+    if (opp_edge) {


it pains me to see this if but i guess its equivalent to logical and anyway

kevinkreiser · 2023-12-02T04:34:38Z

src/thor/multimodal.cc

+  mode_ = travel_mode_t::kPedestrian;
+  travel_type_ = 0;
+  max_walking_dist_ = 0;


why move them out of the initializer list? if it was a warning, just reorder them to shut the warning up

That was more of a preference call, but I can revert. I find it more legible. My eyes have a hard time seeing all variables in long initializer lists

kevinkreiser · 2023-12-02T14:53:33Z

src/thor/dijkstras.cc

@@ -78,7 +78,7 @@ void Dijkstras::Initialize(label_container_t& labels,
  uint32_t edge_label_reservation;
  uint32_t bucket_count;
  GetExpansionHints(bucket_count, edge_label_reservation);
-  labels.reserve(std::min(max_reserved_labels_count_, edge_label_reservation));


this used have the effect of capping the reservation at kInitialEdgeLabelCountDijkstras now you can configure to allow for more. i guess thats ok, but the diff isnt enough to understand thats happening

I can confirm that is what's happening:)

kevinkreiser · 2023-12-02T14:57:42Z

valhalla/thor/costmatrix.h

-  std::vector<baldr::DoubleBucketQueue<sif::BDEdgeLabel>> target_adjacency_;
-  std::vector<std::vector<sif::BDEdgeLabel>> target_edgelabel_;
-  std::vector<EdgeStatus> target_edgestatus_;
+  std::array<std::vector<LocationStatus>, 2> locs_status_;


this type of refactor makes the diff basically impossible to read. i wonder if it would have been better to maybe do this change last or maybe at the top of each method that uses one of these to do something like:

// alias with the old name so we dont see everything as a diff auto& source_adjacency_ = adjacency_[MATRIX_FORW];

and let the names unchanged in the rest of the source. that way we could at least see what changed.

yeah, this wasn't a linear (rather a dirty) process to get here. probably I should have re-written the history before review. however, my hope was that we'll do this review in person (did that with dave though), since 90% of the lines changed and it'd be VERY hard to re-write history to make this better reviewable only looking at diffs.

all good no worries

kevinkreiser · 2023-12-02T15:23:16Z

valhalla/thor/costmatrix.h

  // when doing timezone differencing a timezone cache speeds up the computation
  baldr::DateTime::tz_sys_info_cache_t tz_cache_;

  // for tracking the expansion of the Dijkstra
  expansion_callback_t expansion_callback_;

+  const std::function<void()>* interrupt_ = nullptr;


i appreciate you doing that. its not so hard to add so there is no good excuse why i havent done this for so long..

kevinkreiser · 2023-12-02T15:26:50Z

src/thor/costmatrix.cc

+                                                      kInitialEdgeLabelCountBidirDijkstra)),
+      clear_reserved_memory_(config.get<bool>("clear_reserved_memory", false)), targets_{
+                                                                                    new TargetMap} {
+  // Note, most things are being initialized in Initialize() or before


yeah but there is no gaurantee someone doesnt call a different function that may rely on these before calling sourcestotargets which calls initialize. to avoid any future problems its best to at least initialize these to something other than random values even if we dont intend them to be used (plus it doesnt save anythign to leave them uninitialized)

ok, I'll do that. fwiw, IIRC most algorithms have this problem, not even half the stuff is usually initialized. I'll change it for costmatrix, but will not touch the others for now, the PR is big enough as it stands:)

agree. its like many things. over time we improve, when we touch stuff good to improve it in ways we have found deficient thus far

kevinkreiser · 2023-12-02T15:27:52Z

src/thor/costmatrix.cc

+              // anymore
+              if (targets.empty() && locs_status_[MATRIX_FORW][source].threshold > 0) {
+                // TODO(nils): shouldn't we extend the search here similar to bidir A*
+                //   i.e. if pruning was disabled we extend the search in the other direction


makes intuitive sense

kevinkreiser · 2023-12-02T15:30:02Z

src/thor/costmatrix.cc

+        if (locs_status_[MATRIX_REV][i].threshold == 0) {
+          for (uint32_t source = 0; source < locs_count_[MATRIX_FORW]; source++) {
+            // update the target for each source's remaining targets, if it still exists
+            auto& targets = locs_status_[MATRIX_FORW][source].unfound_connections;


haha cool yeah there you did do the alias thing i was suggesting and it indeed saved some small diffs haha. i still cant really read this without manually comparing each line though

kevinkreiser · 2023-12-02T15:31:11Z

src/thor/costmatrix.cc

  while (true) {
-    // Iterate all target locations in a backwards search


why remove things like this comment? i know its clear to you and me but its nice to have headings on code for newcomers

kevinkreiser · 2023-12-02T15:32:06Z

src/thor/costmatrix.cc

+          // in any case make sure this was the last time we looked at this source
+          locs_status_[MATRIX_FORW][i].threshold = -1;
+          if (locs_remaining_[MATRIX_FORW] > 0) {
+            locs_remaining_[MATRIX_FORW]--;
          }
        }
      }
    }


ok i got to here, am i correct that nothing really changed in either of these loops, was just renaming and comments

correct. the whole costmatrix was 95% pure refactor, the remaining 5% the deadend logic, which is also the only thing I wrote additional tests for. I'd like to add some tests comparing the algos too, making sure they report the same duration/distance (+/- some minor threshold)

kevinkreiser · 2023-12-02T15:47:15Z

src/thor/costmatrix.cc

-    }
-  };
+    // TODO(nils): what if there is a shortcut that supersedes our u-turn? can that even be?
+    // We then need to decide if we should expand the shortcut or the non-shortcut edge...


this is a good question. yes we can make a uturn where a shortcut begins/ends. however, isnt it always safe to expand on the non shortcut? the worst scenario is that we were slightly less efficient in finding a solution because there as a shortcut that we didnt use. maybe thats all you're really saying here: "to be fully optimial we shoudl favor a shortcut uturn (when using shortcuts) as opposed to a superseded edge"

kevinkreiser · 2023-12-02T15:52:24Z

ok im done looking it over! i had a lot of little nitpicks so i'll let you choose which ones you want to do, if you dont feel like doing them but also dont care that much let me know and i can do them.

but the only major thing i would say is we have to do the same perf test/diff for bidir a* since it got some refactoring and edge status changes (which you said you'd already look at).

let me know when done and ill put a ship!

nilsnolde · 2023-12-02T16:15:55Z

Nitpicks are mostly relevant, I'll do them. And check perf/diff on bidir A* next week.

nilsnolde · 2023-12-02T22:21:36Z

@kevinkreiser I addressed your review in 94af58b

nilsnolde · 2023-12-03T15:45:49Z

I diff'd de_benchmark_routes.txt and no difference in either performance or narrative compared to master @kevinkreiser .

Also there were some warnings as errors creeping in again, fixed that too.

nilsnolde marked this pull request as draft October 31, 2023 02:33

save my work on the failing laptop

fddc888

nilsnolde force-pushed the nn-matrix-uturns branch from cc057e4 to fddc888 Compare November 11, 2023 16:08

Merge branch 'master' into nn-matrix-uturns

bf1b3f8

nilsnolde changed the title ~~make costmatrix handle uturns~~ Refactor CostMatrix Nov 12, 2023

nilsnolde added 7 commits November 13, 2023 15:43

refactor successful, all tests pass and the deadend bug is fixed..

b695688

put the expansion direction into the template arguments

ba04dc3

rename vars

ebfaf0b

do the memory retaining stuff as good as possible

469748c

clean up other algos

c7aba13

fix #4360 and the matrix-off-by-1-second issue

f424dce

put the dependencies back in place in cmakelists

4f2a421

nilsnolde force-pushed the nn-matrix-uturns branch from ed214d4 to 4f2a421 Compare November 13, 2023 17:27

nilsnolde marked this pull request as ready for review November 13, 2023 17:28

nilsnolde added 2 commits November 13, 2023 18:30

changelog

a151e29

format

c08774c

nilsnolde requested review from kevinkreiser and dnesbitt61 November 13, 2023 18:09

matrix expansion test also needed an update; good: we expand less edg…

1861861

…es:)

nilsnolde commented Nov 13, 2023

View reviewed changes

test/gurka/test_expansion.cc Outdated Show resolved Hide resolved

nilsnolde mentioned this pull request Nov 13, 2023

More matrix clearing memory #4362

Closed

nilsnolde added 2 commits November 19, 2023 18:56

Merge branch 'master' into nn-matrix-uturns

a00560f

oops, edgecost & transition cost were the same for both trees

70788a8

nilsnolde force-pushed the nn-matrix-uturns branch from 6ffa201 to 70788a8 Compare November 19, 2023 20:42

nilsnolde added 2 commits November 19, 2023 23:38

small update

a8f18cd

ha, what a shitshow.. tiny mis-ordering of statements messed it all up..

65759da

nilsnolde commented Nov 22, 2023

View reviewed changes

src/thor/astar_bss.cc Show resolved Hide resolved

nilsnolde commented Nov 22, 2023

View reviewed changes

src/thor/astar_bss.cc Show resolved Hide resolved

nilsnolde commented Nov 22, 2023

View reviewed changes

src/thor/bidirectional_astar.cc Show resolved Hide resolved

nilsnolde commented Nov 22, 2023

View reviewed changes

src/thor/bidirectional_astar.cc Show resolved Hide resolved

nilsnolde force-pushed the nn-matrix-uturns branch from c9a1a40 to a56ef49 Compare November 30, 2023 19:20

kevinkreiser reviewed Dec 2, 2023

View reviewed changes

address review

94af58b

nilsnolde requested a review from kevinkreiser December 2, 2023 22:20

nilsnolde added 2 commits December 2, 2023 23:21

Merge branch 'master' into nn-matrix-uturns

19e3dae

fix some warnings that crept in

a0963ea

nilsnolde added 2 commits December 4, 2023 09:30

format

4d69943

Merge branch 'master' into nn-matrix-uturns

0213601

nilsnolde mentioned this pull request Dec 4, 2023

add shape to matrix response #4432

Merged

kevinkreiser approved these changes Dec 5, 2023

View reviewed changes

kevinkreiser merged commit 32a09fd into master Dec 5, 2023
6 of 7 checks passed

kevinkreiser deleted the nn-matrix-uturns branch December 5, 2023 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CostMatrix #4372

Refactor CostMatrix #4372

nilsnolde commented Oct 31, 2023 •

edited

Loading

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 2, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 5, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 5, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023 •

edited

Loading

kevinkreiser Dec 2, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 2, 2023 •

edited

Loading

nilsnolde Dec 2, 2023

kevinkreiser Dec 5, 2023

kevinkreiser Dec 2, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 5, 2023

kevinkreiser Dec 2, 2023

kevinkreiser Dec 2, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 2, 2023

nilsnolde Dec 2, 2023

kevinkreiser Dec 2, 2023

kevinkreiser commented Dec 2, 2023

nilsnolde commented Dec 2, 2023

nilsnolde commented Dec 2, 2023

nilsnolde commented Dec 3, 2023

		while (true) {
		// Iterate all target locations in a backwards search

Refactor CostMatrix #4372

Refactor CostMatrix #4372

Conversation

nilsnolde commented Oct 31, 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

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 Dec 2, 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

kevinkreiser Dec 2, 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

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 commented Dec 2, 2023

nilsnolde commented Dec 2, 2023

nilsnolde commented Dec 2, 2023

nilsnolde commented Dec 3, 2023

nilsnolde commented Oct 31, 2023 •

edited

Loading

nilsnolde Dec 2, 2023 •

edited

Loading

kevinkreiser Dec 2, 2023 •

edited

Loading