add time dependent mapmatching output #2030

kevinkreiser · 2019-11-07T01:05:16Z

add time tracking to map matching path formation such that the proper expected time information returned when for example you do trace_route. this will allow the time from regular routes and trace routes to match for the same path at the same time of day.

Co-authored-by: kdiluca kristen.diluca@mapbox.com

…ion information is retained when it is a time dependent map match Co-authored-by: kdiluca <kristen.diluca@mapbox.com>

src/thor/route_matcher.cc

danpaz · 2019-11-07T03:49:52Z

src/thor/map_matcher.cc

+  // We support either the epoch timestamp that came with the trace point or
+  // a local date time which we convert to epoch by finding the first timezone
+  uint32_t origin_epoch = 0;
+  if (options.shape().begin()->time() != -1.0) {


Where does -1.0 come from?

maybe he was checking the default value of time which is -1.0.
Suggestion: provide a bool isValidTime(double time) to make it more obvious.

its the default when you dont specify a timestamp for your gps coordinate, it signifies no timestamp

kevinkreiser · 2019-11-07T12:07:25Z

src/baldr/datetime.cc

@@ -442,6 +442,13 @@ bool is_restricted(const bool type,
  return (dow_in_range && dt_in_range);
 }

+uint32_t second_of_week(uint32_t epoch_time, const NodeInfo* node, double elapsedtime) {


when tracking time within FormPath we need to account for changes in timezone which may change the second of the week that we are on (which changes which speed we get in the edge costing). we add a small helper method to allow us to always get the timezone offset seconds of week after offseting from an epoch time

Could you add this comment as a docstring for the function?

it is in the header we dont usually copy those to the impl

kevinkreiser · 2019-11-07T12:10:05Z

src/thor/map_matcher.cc

+  if (options.shape().begin()->time() != -1.0) {
+    origin_epoch = options.shape().begin()->time();
+  } else if (options.shape().begin()->has_date_time()) {
+    for (const auto& s : edge_segments) {


here we allow also the date time string as an input for the time of the route. the trick is we need a nodeinfo to get the timezone so we can get the epoch time from it. so we just find the first one and use it. this may be somewhat inaccurate if the beginning of the match has discontinuities but it is less likely that you are flip flopping timezones.

kevinkreiser · 2019-11-07T12:12:01Z

src/thor/map_matcher.cc

@@ -153,39 +173,43 @@ MapMatcher::FormPath(meili::MapMatcher* matcher,
      // This means there is a discontinuity. If so, fallback to using costing
      use_timestamps = false;
    }
-    last_interp_index = interpolations.front().back().original_index;
+    if (!interpolations.empty())
+      last_interp_index = interpolations.front().back().original_index;


@kdiluca i dont remember making this change can you speak to it? i can see its to protect against dereferencing something that doesnt exist but it defaults to an index of 0 so maybe its not all the protection we need. have to look below i guess.

ok ive rejiggered thsi to avoid any possible access of interpolations that doesnt make sense

kevinkreiser · 2019-11-07T12:23:16Z

src/thor/map_matcher.cc

+    // Set seconds from beginning of the week
+    uint32_t second_of_week = kInvalidSecondsOfWeek;
+    // have to always compute the offset in case the timezone changes along the path
+    // we could cache the timezone and just add seconds when the timezone doesnt change
+    // we wont have a valid node on the first pass but then again we also wont have any
+    // elapsed time so this would be irrelevant anyway
+    if (origin_epoch != 0 && nodeinfo) {
+      second_of_week = DateTime::second_of_week(origin_epoch, nodeinfo, elapsed.secs);
    }


this is the meat of the change. on every new path edge we figure out what second of the week it was (if it was a time dependent route) and use that to do the edge costing below. thsi allows the costing to access the rigth speed for the time of day that route was supposed to have been happening

kevinkreiser · 2019-11-07T12:23:50Z

src/thor/route_matcher.cc

@@ -1,3 +1,4 @@
+#include "baldr/datetime.h"


the changes in this file are basically the same as in map_matcher in spirit

kevinkreiser · 2019-11-07T12:24:36Z

src/thor/route_matcher.cc

        if (use_timestamps)
-          elapsed.secs = shape[index].epoch_time() - shape[0].epoch_time();
+          elapsed.secs = shape.back().epoch_time() - shape[0].epoch_time();


this looks like an unintended change @kdiluca, will revert

kevinkreiser · 2019-11-07T12:28:28Z

src/thor/route_matcher.cc

-          // Match failed along this edge, pop the last entry off path_infos and try the next edge
+          // Match failed along this edge, pop the last entry off path_infos as well as what it
+          // contributed to the elapsed cost/time and try to keep going on the next edge
+          elapsed -= cost;


previously when we went down a path that didnt match we weren't removing the impact it had on the elapsed time. this should take care of that and was the only reason i broke out the cost into its own object above. another option would ahve been to look at the previous path info if any and do the subtracting from there

kevinkreiser · 2019-11-07T12:29:54Z

src/thor/route_matcher.cc

@@ -288,6 +330,7 @@ bool RouteMatcher::FormPath(const std::shared_ptr<DynamicCost>* mode_costing,
    float de_remaining_length = de->length() * (1 - edge.percent_along());
    float de_length = length_comparison(de_remaining_length, true);
    EdgeLabel prev_edge_label;
+    Cost elapsed;


the elapsed cost is in the inner loop because the outer loop is in case a potential edge candidate fails. again we dont want to count that failures elapsed time toward the overall correct match so we reset for each candidate path we try

kevinkreiser · 2019-11-07T12:32:27Z

src/thor/triplegbuilder.cc

-      second_of_week =
-          DateTime::day_of_week(dt) * kSecondsPerDay + DateTime::seconds_from_midnight(dt);
+    if (origin_epoch != 0) {
+      second_of_week = DateTime::second_of_week(origin_epoch, node, elapsedtime);


this was already working but now that we are doing it in other places, we refactored it out into a function and now can use it here as well

kevinkreiser · 2019-11-07T12:33:14Z

test/mapmatch.cc

+std::string to_locations(const std::vector<PointLL>& shape,
+                         const std::vector<float>& accuracies,
+                         int frequency = 0,
+                         int start_time = 8 * 60 * 60) {


when generating locations for these tests its nice to be able to set a start time that isnt 0

kevinkreiser · 2019-11-07T12:33:50Z

test/mapmatch.cc

    try {
      walked_json = actor.trace_attributes(
-          R"({"costing":"auto","shape_match":"edge_walk","encoded_polyline":")" +
+          R"({"date_time":{"type":1,"value":"2019-10-31T18:30"},"costing":"auto","shape_match":"edge_walk","encoded_polyline":")" +


for every test we first do an edge walk match with a date time on it to check that that code path works

and then for the regular map match we are using the to_locations function above to set timestamps on every resampled noisy gps point to trigger timestamp matching in that path way.

yuzheyan · 2019-11-07T04:27:48Z

src/thor/map_matcher.cc

+  // We support either the epoch timestamp that came with the trace point or
+  // a local date time which we convert to epoch by finding the first timezone
+  uint32_t origin_epoch = 0;
+  if (options.shape().begin()->time() != -1.0) {


maybe he was checking the default value of time which is -1.0.
Suggestion: provide a bool isValidTime(double time) to make it more obvious.

yuzheyan · 2019-11-07T04:33:21Z

src/thor/map_matcher.cc

+    origin_epoch = options.shape().begin()->time();
+  } else if (options.shape().begin()->has_date_time()) {
+    for (const auto& s : edge_segments) {
+      if (!s.edgeid.Is_Valid() || !matcher->graphreader().GetGraphTile(s.edgeid, tile))


matching->graphreader().GetGraphTile(s.edgeid, tile) == nullptr

yuzheyan · 2019-11-07T04:34:35Z

src/thor/map_matcher.cc

+      if (!s.edgeid.Is_Valid() || !matcher->graphreader().GetGraphTile(s.edgeid, tile))
+        continue;
+      directededge = tile->directededge(s.edgeid);
+      if (matcher->graphreader().GetGraphTile(directededge->endnode(), tile)) {


check != nullptr

src/thor/map_matcher.cc

yuzheyan · 2019-11-07T04:40:18Z

src/thor/map_matcher.cc

+    // we could cache the timezone and just add seconds when the timezone doesnt change
+    // we wont have a valid node on the first pass but then again we also wont have any
+    // elapsed time so this would be irrelevant anyway
+    if (origin_epoch != 0 && nodeinfo) {


nodeinfo != nullptr

yuzheyan · 2019-11-07T05:03:19Z

src/valhalla_run_route.cc

@@ -211,8 +211,7 @@ const valhalla::TripLeg* PathTest(GraphReader& reader,
      PathLocation::toPBF(path_location.back(), options.mutable_locations()->Add(), reader);
    }
    std::vector<PathInfo> path;
-    bool ret =
-        RouteMatcher::FormPath(mode_costing, mode, reader, trace, false, options.locations(), path);
+    bool ret = RouteMatcher::FormPath(mode_costing, mode, reader, trace, options, path);


need better name for ret

yuzheyan · 2019-11-07T05:06:39Z

src/thor/route_matcher.cc

-          // Match failed along this edge, pop the last entry off path_infos and try the next edge
+          // Match failed along this edge, pop the last entry off path_infos as well as what it
+          // contributed to the elapsed cost/time and try to keep going on the next edge
+          elapsed -= cost;


elapsed can not go negative, maybe need a check negative exception to catch bugs.

so above we add cost on, and here we subtract it back off. logically you can say it would only go negative if floating point round off screwed it up and puts it just below 0 HOWEVER we are inside the node expansion function which means we've already previously added the first edge on the path, ie the elapsed cost was already non zero, ie not close to zero when we entered this function. because of that i dont see how its possible for it to ever go below zero (unless someone writes a costing for which an edge has 0 cost, however this would cause many worse problems before making it to this point)

yuzheyan · 2019-11-07T05:07:28Z

src/thor/route_matcher.cc

-    for (uint32_t i = start_trans; i < node_info->transition_count(); ++i, ++trans) {
+  if (!from_transition && nodeinfo->transition_count() > 0) {
+    const NodeTransition* trans = tile->transition(nodeinfo->transition_index());
+    for (uint32_t i = start_trans; i < nodeinfo->transition_count(); ++i, ++trans) {


optimize away node0>transition_count() call in the for loop

yuzheyan · 2019-11-07T05:09:08Z

src/thor/route_matcher.cc

+          elapsed +=
+              costing->EdgeCost(end_de, end_edge_tile, second_of_week) * end_edge.percent_along();
+          // overwrite time with timestamps
+          if (options.use_timestamps())


add {} for single line if condition

yuzheyan · 2019-11-07T05:52:26Z

src/thor/map_matcher.cc

+        nodeinfo = nullptr;
+      }
+    }
+  }

  // Interpolate match results if using timestamps for elapsed time
  std::list<std::vector<interpolation_t>> interpolations;


using of std::list is suspicious. Rule of thumb, unless we need to perform list.splice() to do constant time re-ordering and such operations happens a lot. We should always avoid using std::list. Normally, reader would assume the code will often perform splice/ or delete element from middle(when container is huge, otherwise vector always wins in speed).

alternatives are std::deque/vector depends on if the size is know before hand, how many reallocations will be performed and how often do we delete items in the middle.

yeah i agree with this i just tend to not refactor everything if the PR that im working on isnt specifically about that.

…set the epoch time when you specify a date_time

… second of week even in the case of epoch time being provided

…lts when timezone is included

…ts to verify

kevinkreiser · 2019-11-08T15:01:17Z

README.md

@@ -25,7 +25,7 @@ Build Status

 | Linux/MacOs | Windows | Code Coverage |
 | ----------- | ------- | ------------- |
-| [![Circle CI](https://circleci.com/gh/valhalla/valhalla.svg?style=svg)](https://circleci.com/gh/valhalla/valhalla) | [![Build status](https://ci.appveyor.com/api/projects/status/6w7emulgcjweu457/branch/master?svg=true)](https://ci.appveyor.com/project/kevinkreiser/valhalla/branch/master) | [![codecov](https://codecov.io/gh/valhalla/valhalla/branch/master/graph/badge.svg)](https://codecov.io/gh/valhalla/valhalla) |
+| [![Circle CI](https://circleci.com/gh/valhalla/valhalla.svg?style=svg)](https://circleci.com/gh/valhalla/valhalla) | [![Build Status](https://dev.azure.com/valhalla1/valhalla/_apis/build/status/valhalla.valhalla?branchName=master)](https://dev.azure.com/valhalla1/valhalla/_build/latest?definitionId=1&branchName=master) | [![codecov](https://codecov.io/gh/valhalla/valhalla/branch/master/graph/badge.svg)](https://codecov.io/gh/valhalla/valhalla) |


@mloskot i switched over to the azure devops pipeline for windows builds, although they seem to take appx as long (due to installing dependencies) they start sooner and have more capacity to run parallel builds so the end result is a build that actually finishes in time for the rest of the platforms' ci's

…lder matches what mapmatching is doing in terms of sequence of operations. also fix issue where predicted flow is disabled for map matching when only using timestamps

yuzheyan

some unimportant readability comments.

yuzheyan · 2019-11-08T16:54:34Z

src/baldr/datetime.cc

  const auto date = date::make_zoned(time_zone, tp);
-  iso_date_time << date::format("%FT%R%z", date);
+


why don't combine two the if(tz_format) together?
tz_format means has_tz_format right?

yuzheyan · 2019-11-08T16:57:09Z

src/thor/map_matcher.cc

+      nodeinfo = tile->node(directededge->endnode());
+      const auto* tz = DateTime::get_tz_db().from_index(nodeinfo->timezone());
+      // if its timestamp based need to signal that out to trip leg builder
+      if (options.shape(0).time() != -1.0) {


adding a is_valid_time(double time) function maybe better for time validation check.

yeah, maybe to avoid confusion it should take the api::Location as an argument. the reason being is that this is gnerated code so the only way to actually do this would be with a free function which of course leads to some ambiguity imho

yuzheyan · 2019-11-08T17:02:50Z

src/thor/route_matcher.cc

@@ -364,7 +427,7 @@ bool RouteMatcher::FormPath(const std::shared_ptr<DynamicCost>* mode_costing,
        // Update the elapsed time based on edge cost
        elapsed += mode_costing[static_cast<int>(mode)]->EdgeCost(de, end_node_tile) *
                   (end.second.first.percent_along() - edge.percent_along());
-        if (use_timestamps)
+        if (options.use_timestamps())


for single line if statement, sometimes it has {} sometimes it does not, maybe it is better to enforce it with {}?

yuzheyan · 2019-11-08T17:05:22Z

src/thor/route_matcher.cc

@@ -277,7 +319,7 @@ bool RouteMatcher::FormPath(const std::shared_ptr<DynamicCost>* mode_costing,
    // Process directed edge and info
    const DirectedEdge* de = begin_edge_tile->directededge(graphid);
    const GraphTile* end_node_tile = reader.GetGraphTile(de->endnode());
-    if (begin_edge_tile == nullptr) {
+    if (end_node_tile == nullptr) {


what is the rule of nullptr checking? Maybe it is better to have consistency by enforce compare with nullptr?

yuzheyan · 2019-11-08T17:07:16Z

src/thor/trace_route_action.cc

+          // because triplegbuilder relies on it. form path set the first one but
+          // on the subsequent legs we will need to set them by doing time offsetting
+          // like is done in route_action.cc thor_worker_t::depart_at
+


yuzheyan · 2019-11-08T17:09:08Z

src/worker.cc

@@ -448,6 +451,23 @@ void parse_locations(const rapidjson::Document& doc,
      midgard::logging::Log(node + "_count::" + std::to_string(request_locations->Size()),
                            " [ANALYTICS] ");
    }
+
+    // push the date time information down into the locations
+    if (!had_date_time && options.has_date_time() && locations->size()) {


!locations->empty()

this is pbf generated code, pbf did not generate empty for some older versions so to maintain compatibility we use size

yuzheyan · 2019-11-08T17:11:35Z

valhalla/thor/route_matcher.h

@@ -39,8 +39,7 @@ class RouteMatcher {
                       const sif::TravelMode& mode,
                       baldr::GraphReader& reader,
                       const std::vector<meili::Measurement>& shape,
-                       const bool use_timestamps,
-                       const google::protobuf::RepeatedPtrField<valhalla::Location>& correlated,
+                       valhalla::Options& options,


it is somehow against intuitive to pass options as & then modify it. Maybe need another name or way.

the problem is that we need to set time information on the locations. this is related to the TODOs i added in the functions that call these map matching FromPath functions. we could move all of the date setting there but that is a bit hairy because we are supporting timestamps as a date which means we need to get a node to turn it into tz and then turn the epoch into date string. the whole thing could probably be refactored to be easier to understand.

as it stands though this is inline with the request flow throughout the process. essentially as we fulfill the request we fill in bits of info along the way, which all hangs off of the api object, which owns this options object.

kevinkreiser · 2019-11-08T17:37:06Z

@yuzheyan there seem to be a couple things going on here wrt to style etc. let me explain to you my principles when making a pr:

i try not to change too much prior code unless its directly impacting the functionaly im changing. so if someone had extra braces on a single line if, i let them there, if someone already had == nullptr i let it there. i seek to make as minimal a diff as possible to make it easier to understand what i changed vs what i didnt. i may, as you have done, see places where old code could be more efficient, generally tend to just leave it untouched if it isnt part of what my PR is addressing. at the most i might add a TODO comment mentioning a more efficient change that could be done.
we do not currently have very strict styling rules. we run a linter which requires some formatting. if we want to we can make that more strict. i personally prefer leaving out extra curly braces and checks for nullptr as i find those pedantic but i understand that everyone has their own opinion. except for tab indents those are universally hated :)

yuzheyan · 2019-11-08T17:48:49Z

Thanks for the explanation, agreed! Sorry for being annoying of picking cherries. I will avoid doing that until we have a style guid. Tiny things builds up quick especially in algorithm development, and we have multi contributors on the same project, many factors.

…

On Fri, Nov 8, 2019 at 9:40 AM Kevin Kreiser ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/worker.cc <#2030 (comment)>: > @@ -448,6 +451,23 @@ void parse_locations(const rapidjson::Document& doc, midgard::logging::Log(node + "_count::" + std::to_string(request_locations->Size()), " [ANALYTICS] "); } + + // push the date time information down into the locations + if (!had_date_time && options.has_date_time() && locations->size()) { this is pbf generated code, pbf did not generate empty for some older versions so to maintain compatibility we use size — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2030?email_source=notifications&email_token=AEAYSXUKJ2DE73IW4TI4VF3QSWP7FA5CNFSM4JJ75VJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCK6K5KQ#discussion_r344289021>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEAYSXQJGEGM36QHSJ7WGFTQSWP7FANCNFSM4JJ75VJA> .

kevinkreiser · 2019-11-08T18:03:11Z

@yuzheyan would you like to take a swag at some changes to the style guide? i agree with all the different contributors that its gotten to a state of eclecticness that might be too far..

would you mind starting a new issue to tackle this concept and make some proposals. It would be great if most of it could be done via clang format however i imagine more semantic things like == nullptr are not possible (though maybe with clang tidy?)

kevinkreiser · 2019-11-08T21:49:48Z

im still trying to bump the coverage on this.

…racking

danpaz

Some ideas to add to the tests. All else LGTM.

danpaz · 2019-11-11T16:20:19Z

test/mapmatch.cc

+           {"lat":52.0796213915245,"lon":5.139262676239015,"accuracy":10,"time":8},
+           {"lat":52.079637875461195,"lon":5.139581859111787,"accuracy":10,"time":10},
+           {"lat":52.07964776582031,"lon":5.139828622341157,"accuracy":10,"time":12},
+           {"lat":52.07985600778458,"lon":5.1404121782302865,"accuracy":10,"time":14}]})"));


I see you added time to all these test cases. Should we keep one or two test cases without timestamps to test the default case?

Is it possible also to add an assertion on the returned duration? At least make sure it's non-zero / non-negative?

add time tracking to map matching such that the proper expected durat…

4db18c2

…ion information is retained when it is a time dependent map match Co-authored-by: kdiluca <kristen.diluca@mapbox.com>

kevinkreiser requested review from yuzheyan and danpaz November 7, 2019 01:05

danpaz reviewed Nov 7, 2019

View reviewed changes

lint

a827584

kevinkreiser commented Nov 7, 2019

View reviewed changes

avoid possible wrong access

04a7dda

kevinkreiser commented Nov 7, 2019

View reviewed changes

revert accident copy paste fail

c65abaf

kevinkreiser commented Nov 7, 2019

View reviewed changes

kevinkreiser added 2 commits November 7, 2019 10:22

have dirent headers available for all targets

fab0e33

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

0fba54f

yuzheyan reviewed Nov 7, 2019

View reviewed changes

kevinkreiser added 5 commits November 7, 2019 13:44

refactor time parsing so it makes it to all locations. also actually …

f678a2c

…set the epoch time when you specify a date_time

actually we always need a node to get timezone so we can always get a…

71b2439

… second of week even in the case of epoch time being provided

oops wrong index

f7857d3

add azure build badge. fix use of date time parsing to avoid bad resu…

307436f

…lts when timezone is included

update second of week check to account for actual seconds and add tes…

ba9b0db

…ts to verify

kevinkreiser commented Nov 8, 2019

View reviewed changes

move the node costing after the second day check so taht trip leg bui…

cac58c1

…lder matches what mapmatching is doing in terms of sequence of operations. also fix issue where predicted flow is disabled for map matching when only using timestamps

yuzheyan reviewed Nov 8, 2019

View reviewed changes

kevinkreiser added 3 commits November 8, 2019 16:49

Merge branch 'master' into mm_time_tracking

8a4776e

increase coverage for datestamp in the encoded polyline case

f102939

Merge remote-tracking branch 'origin/mm_time_tracking' into mm_time_t…

d4551c7

…racking

danpaz previously approved these changes Nov 11, 2019

View reviewed changes

add more test coverage

8955b68

kevinkreiser dismissed danpaz’s stale review via 8955b68 November 11, 2019 19:59

Merge branch 'master' into mm_time_tracking

ffe1ca7

danpaz approved these changes Nov 11, 2019

View reviewed changes

kevinkreiser merged commit 7bcca3f into master Nov 12, 2019

kevinkreiser mentioned this pull request Nov 15, 2019

fix current time map matches #2060

Merged

purew deleted the mm_time_tracking branch November 26, 2019 15:23

		const auto date = date::make_zoned(time_zone, tp);
		iso_date_time << date::format("%FT%R%z", date);

add time dependent mapmatching output #2030

add time dependent mapmatching output #2030

Conversation

kevinkreiser commented Nov 7, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzheyan left a 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 Nov 8, 2019

yuzheyan commented Nov 8, 2019 via email

kevinkreiser commented Nov 8, 2019

kevinkreiser commented Nov 8, 2019

danpaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment