Unify Time Tracking in all the Algorithms #2278

kevinkreiser · 2020-03-18T18:51:23Z

Issue

This PR seeks to unify the way we track time during any given algorithms expansion in the graph. At the moment we use a bunch of the same patterns in all the different algorithms, and it would be great to encapsulate that into a single set of functionality that all the different code paths can re-use.

The eventual goal of this is to do bidirectional a* with time dependence but that functionality will be a completely separate PR. this is just the first steps

Tasklist

create and use struct in all algorithms
add struct unit tests
add integration (route) unit tests
Update the changelog

…tras to go

…es that we could maybe move later

kevinkreiser · 2020-03-24T16:00:06Z

valhalla/baldr/datetime.h

-      t.tm_hour < 0 || t.tm_hour > 23 || t.tm_min < 0 || t.tm_min > 59) {
-    t.tm_year = 0;
+  // If parsing failed zero 0 the struct
+  if (ss.fail()) {


@gknisely you can more easily test for failure by checking the failbit on the stream

kevinkreiser · 2020-03-24T16:01:26Z

src/baldr/datetime.cc

+
+  // we weren't able to use this string as a date and you'd like to know about it
+  if (can_throw && in.fail())
+    throw std::invalid_argument("Date string is invalid: " + date);


@gknisely if the caller says you can throw then we throw when they pass an invalid date format. the default is to NOT throw which is how the code was previously working. currently only the new code ive written for time tracking asks this to throw. i dont like it but i dont want to go through the exorcise right now of making all the places that call this catch

@kevinkreiser That works, but can we create an issue for this with examples of the bad input so that we can update and create tests. Thank you.

…. add increment and decrement operator tests

kevinkreiser · 2020-03-25T15:40:36Z

src/baldr/graphreader.cc

@@ -851,5 +851,10 @@ AABB2<PointLL> GraphReader::GetMinimumBoundingBox(const AABB2<PointLL>& bb) {
  return min_bb;
 }

+int GraphReader::GetTimezone(const baldr::GraphId& node, const GraphTile*& tile) {


this was a helper funciton in the timedependent algorithms but since all of them need to use it i figured we can move it to the graphreader where we have other helpers

kevinkreiser · 2020-03-25T15:40:58Z

src/thor/CMakeLists.txt

@@ -2,26 +2,27 @@ file(GLOB headers ${VALHALLA_SOURCE_DIR}/valhalla/thor/*.h)

 set(sources
  astar.cc
+  attributes_controller.cc


i went to insert the new source file and saw this wasnt alphabetially sorted, so i did that

kevinkreiser · 2020-03-25T15:41:31Z

src/thor/astar.cc

-  if (closest_ni != nullptr && origin.has_date_time() && origin.date_time() == "current") {
-    origin.set_date_time(
-        DateTime::iso_date_time(DateTime::get_tz_db().from_index(closest_ni->timezone())));
-  }


i gutted all the time setting stuff from all the algorithms becuse the new struct handles all that for you

kevinkreiser · 2020-03-25T15:41:55Z

src/thor/bidirectional_astar.cc

@@ -43,7 +43,6 @@ BidirectionalAStar::BidirectionalAStar() : PathAlgorithm() {

 // Destructor
 BidirectionalAStar::~BidirectionalAStar() {
-  Clear();


this was pointless, but the class isnt final so i left the destructor

kevinkreiser · 2020-03-25T15:42:41Z

src/thor/bidirectional_astar.cc

@@ -113,7 +112,8 @@ bool BidirectionalAStar::ExpandForward(GraphReader& graphreader,
                                       const GraphId& node,
                                       BDEdgeLabel& pred,
                                       const uint32_t pred_idx,
-                                       const bool from_transition) {
+                                       const bool from_transition,
+                                       const TimeInfo& time_info) {


all the algorithms pass this struct around to compute time offsets at every expansion. instead of having the guts of that computation in the algorithm, its all in the TimeInfo struct nicely encapsulated

kevinkreiser · 2020-03-25T15:43:27Z

src/thor/bidirectional_astar.cc

@@ -228,7 +234,8 @@ inline bool BidirectionalAStar::ExpandForwardInner(GraphReader& graphreader,
    return true; // This is an edge we _could_ have expanded, so return true
  }

-  const uint64_t localtime = 0; // Bidirectional is not yet time-aware
+  // TODO: actually use time_info


we're not actually to the point yet where bidirectional makes time aware expansion choices. coming soon

…so use it from the tests

kevinkreiser · 2020-05-28T21:50:06Z

Ok I've brought this branch back up to speed in both senses of the word. I've made use of the caching of timezones and I've gotten master merged back into it. I've added extensive testing of the new little struct and done benchmarking using a file of 13797 large and small routes all over the world. I used the run_with_server.py script with an mmap tar of the planet. Both the server and the script were set to concurrency=24.

Master completed the routes in:

real	0m38.098s
user	0m28.145s
sys	0m2.287s

Where as this branch completed the routes in:

real	0m38.107s
user	0m28.569s
sys	0m2.309s

A difference of just 9 milliseconds which is well within the margin of error over the course of almost 14k route requests.

kevinkreiser · 2020-05-28T21:56:26Z

valhalla/baldr/datetime.h

-    const date::time_zone* origin_tz,
-    const date::time_zone* dest_tz,
-    std::unordered_map<const date::time_zone*, std::vector<date::sys_info>>* cache = nullptr);
+using tz_sys_info_cache_t = std::unordered_map<const date::time_zone*, std::vector<date::sys_info>>;


i couldnt stand looking at this detail so i type aliased it away

kevinkreiser · 2020-05-28T21:58:44Z

valhalla/baldr/time_info.h

+   * @return the initialized TimeInfo
+   */
+  static TimeInfo
+  make(valhalla::Location& location,


all of the algorithms now start off by calling this function. it sets all the time information that we need to track over the course of the expansion, see the members above.

kevinkreiser · 2020-05-28T22:00:22Z

valhalla/baldr/time_info.h

+   * @param next_tz_index   the timezone index at the new location
+   * @return a new TimeInfo object reflecting the offset
+   */
+  inline TimeInfo forward(float seconds_offset, int next_tz_index) const {


this method and its sister method below are in charge of updating the timeinfo object to reflect the passage of time as expansion proceeds. so after you traverse an edge you provide the amount of time that passed during that traversal to this function so it can update its members to reflect that time change.

kevinkreiser · 2020-05-28T22:03:36Z

valhalla/baldr/time_info.h

+namespace valhalla {
+namespace baldr {
+// A structure for tracking time information as the route progresses
+struct TimeInfo {


the main point of this struct is to provide a one stop shop for 2 things:

convenient encapsulation that allows any routing algorithm (graph expansion) to track time

parameter pack that we can, in the future, send directly to costing to do the right thign with respect to time-based speed look up (second of week for historical speed and seconds from now for real time speed)

purew · 2020-05-29T14:28:28Z

test/gurka/test_time_tracking.cc

+  const gurka::ways ways = {{"AB", {{"highway", "trunk"}}}};
+  const auto layout = gurka::detail::map_to_coordinates(ascii_map, 100);
+  auto map = gurka::buildtiles(layout, ways, {}, {}, "test/data/gurka_time_tracking_make",
+                               {{"mjlonir.timezone", "/path/to/timezone.sqlite"}});


mjlonir? :)

purew · 2020-05-29T15:02:06Z

test/gurka/test_time_tracking.cc

+      }
+    }
+  }
+  std::vector<double> expected = {0, 17.1429, 34.2857, 176.789, 220.289, 244.289, 268.289};


Why the big step between 34s to 176s? Is it because of a huge turncost when turning south?

i want to say its because its right hand driving an you make a left onto a lower class road from the ramp but... what i really did here was run this exact test on master and captured the output. this was basically a regression test

…te changelog

purew · 2020-05-29T15:37:50Z

valhalla/baldr/time_info.h

+      sw += tz_diff;
+    }
+
+    // wrap the week second if it went past the end


Could the seconds_of_week go past the beginning? I.e, become <0?

not in this function, but it could in the function below which checks the other direction

@purew pointed out that tz_diff is signed. so actually hes right! technically it should all be signed and get wrapped in either direction in both functions. this is important in the forward direction when you drive west over a timezone but are routing near midnight on sunday

purew · 2020-05-29T15:46:18Z

valhalla/baldr/time_info.h

+  uint64_t seconds_from_now : 43;
+
+  // the sign bit for seconds_from_now
+  uint64_t negative_seconds_from_now : 1;


Could this be named something to signify it's a boolean, like is_negative_... or has_negative_...?

purew

LGTM Great with all the unification! 🍰

Just had a few comments

purew

GR8!

kevinkreiser · 2020-06-01T14:27:54Z

So I've now also tested timedep routes and the main difference is that now that we are using the timezone cache, running the 14k routes from above finishes in real 3m12.889s where as the old code finished in real 16m22.108s. this is still a far cry from bidirectionals 40 seconds but its a nice improvement. There were slight differences in timing though. about half of them had a difference of 1 second which i dont care about at all. About 1% also had slight deviations in the path chosen but had arrival (depart_at) times that worked out to be the same (minute resolution) which i also chaulk up to having that 1 second differences (they were usually long routes with a tiny deviation from the previous codes path). We could try to reconcile the 1 second difference but I dont think its needed.

kevinkreiser added 5 commits March 12, 2020 15:32

move convenience function to get timezone index to graphreader

762a1e6

add time tracking to bidir a*. dont affect the routes though yet

719035e

forward and reverse both use the time tracking. multi modal and dijks…

1b27ba0

…tras to go

convert multimodal. it has a bunch of other stuff specific to schedul…

6339868

…es that we could maybe move later

use time info struct to track time in dijkstras

830f16a

kevinkreiser changed the title ~~Timedep bidir astar~~ Unify Time Tracking in all the Algorithms Mar 18, 2020

kevinkreiser added 8 commits March 18, 2020 14:53

forgot the one new source file

ca2744c

date parsing hardening

101972e

for now ignore errors..

67bb74d

default the timezone when there is none, remove boolean cast

7b359e4

tidy

94b7778

cant use on the fly unique pointer as it goes out of scope

37cd5f8

add tests for timeinfo make function

4889b57

finish timeinfo::make function testing

47fa0d6

kevinkreiser marked this pull request as ready for review March 24, 2020 15:58

kevinkreiser requested review from purew, danpat and dnesbitt61 March 24, 2020 15:58

kevinkreiser commented Mar 24, 2020

View reviewed changes

kevinkreiser added 3 commits March 24, 2020 17:41

dont need current boolean we track use of current flow in the costing…

b506fc5

…. add increment and decrement operator tests

allow overriding parts of the config created by gurka

16f3eb4

Merge branch 'master' into timedep_bidir_astar

148f1c7

kevinkreiser requested a review from danpaz March 25, 2020 15:38

kevinkreiser commented Mar 25, 2020

View reviewed changes

kevinkreiser added 2 commits April 16, 2020 13:46

try ccache for mac

ce08c0b

Merge branch 'master' into timedep_bidir_astar

2da75c9

coxchapman added this to the Sprint 11 - Left Turn milestone May 26, 2020

kevinkreiser added 5 commits May 26, 2020 21:55

resolve conflicts

0b416b2

remove leap second testing

a6b9c79

move time info into its own header and handle negative offsets from now

a07fec4

add timezone cache to timeinfo and use it with all the algorithms. al…

0251200

…so use it from the tests

Merge remote-tracking branch 'origin/master' into timedep_bidir_astar

46147ea

kevinkreiser commented May 28, 2020

View reviewed changes

purew reviewed May 29, 2020

View reviewed changes

fix typo in test, actually stop the route request python script, upda…

435096d

…te changelog

purew reviewed May 29, 2020

View reviewed changes

purew previously approved these changes May 29, 2020

View reviewed changes

add test that exposes wrapping due to timezone offset and fix

49655bf

kevinkreiser dismissed purew’s stale review via 49655bf May 29, 2020 16:50

purew previously approved these changes May 29, 2020

View reviewed changes

fix test of seconds from now to be robust to different times of day

87ff3d1

kevinkreiser dismissed purew’s stale review via 87ff3d1 May 30, 2020 02:16

Merge branch 'master' into timedep_bidir_astar

4f77946

purew approved these changes Jun 1, 2020

View reviewed changes

kevinkreiser merged commit 6f6e29e into master Jun 1, 2020

purew deleted the timedep_bidir_astar branch June 1, 2020 18:26

kevinkreiser mentioned this pull request Dec 2, 2020

remove astar since its redundant with timedep_forward #2706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Time Tracking in all the Algorithms #2278

Unify Time Tracking in all the Algorithms #2278

kevinkreiser commented Mar 18, 2020 •

edited

Loading

kevinkreiser Mar 24, 2020

kevinkreiser Mar 24, 2020

gknisely Mar 24, 2020

kevinkreiser Mar 25, 2020

kevinkreiser Mar 25, 2020

kevinkreiser Mar 25, 2020

kevinkreiser Mar 25, 2020

kevinkreiser Mar 25, 2020 •

edited

Loading

kevinkreiser Mar 25, 2020

kevinkreiser commented May 28, 2020

kevinkreiser May 28, 2020

kevinkreiser May 28, 2020

kevinkreiser May 28, 2020

kevinkreiser May 28, 2020

purew May 29, 2020

purew May 29, 2020

kevinkreiser May 29, 2020

purew May 29, 2020

kevinkreiser May 29, 2020

kevinkreiser May 29, 2020

purew May 29, 2020

purew left a comment

purew left a comment

kevinkreiser commented Jun 1, 2020

Unify Time Tracking in all the Algorithms #2278

Unify Time Tracking in all the Algorithms #2278

Conversation

kevinkreiser commented Mar 18, 2020 • edited Loading

Issue

Tasklist

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 Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented May 28, 2020

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

purew left a comment

Choose a reason for hiding this comment

purew left a comment

Choose a reason for hiding this comment

kevinkreiser commented Jun 1, 2020

kevinkreiser commented Mar 18, 2020 •

edited

Loading

kevinkreiser Mar 25, 2020 •

edited

Loading