Fixes bug with inverted time-restriction parsing #2167

purew · 2020-01-16T23:02:18Z

The time-restriction parsing had a logic bug that meant permission
was allowed at restricted times and vice versa.

This commit also adds a unit-test to verify this functionality.

mandeepsandhu · 2020-01-17T00:02:04Z

valhalla/mjolnir/complexrestrictionbuilder.h

@@ -145,7 +145,8 @@ class ComplexRestrictionBuilder : public baldr::ComplexRestriction {

  /**
   * Set the dow mask.  indicates days of week to apply the restriction
-   * @param  dow  day of week - This is a mask (e.g., Mo-Fr = 62).
+   * @param  dow  day of week - This is a mask with first day of week being sunday e.g., Mo-Fr = 62
+   * (111110).


I think showing 1 bit for each day of week here will be a tad more helpful (0111110). Its easy to correlate days to bits then. Also, can we state which bit is Sunday, the MSB or LSB?

👍 @mandeepsandhu Updated

purew · 2020-01-17T14:14:50Z

~~Hmm found something wonky with the logic. Investigating.~~

Ah it's not testing the kTimeAllowed branch. Let me fix that.

kdiluca · 2020-01-17T15:40:06Z

👍 Would love to get this merged so that I can pull into my branch! Thanks @purew

(test) Logic still broken, working on fix

purew · 2020-01-17T20:31:23Z

src/baldr/datetime.cc

-                   const uint8_t end_day_dow,
-                   const uint64_t current_time,
-                   const date::time_zone* time_zone) {
+bool is_conditional_active(const bool type,


I renamed this function to better portray what it is actually doing.

purew · 2020-01-17T20:31:44Z

test/CMakeLists.txt

@@ -78,8 +78,9 @@ add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/test/data/whitelion_tiles_reverse/
 add_custom_target(reversed_whitelion_tiles DEPENDS ${CMAKE_BINARY_DIR}/test/data/whitelion_tiles_reverse/2/000/814/309.gph)

 add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/test/data/roma_tiles/1/047/352.gph
+  COMMAND ${VALHALLA_SOURCE_DIR}/scripts/valhalla_build_timezones 2>/dev/null > test/data/roma_tiles/tz.sqlite


Ensure we add timezone information to the Roma tiles

purew · 2020-01-17T20:32:32Z

test/astar.cc

+  //    <tag k="auto" v="yes"/>
+  //    <tag k="motor_vehicle:conditional" v="no @ (Mo-Sa 07:00-16:00)"/>
+  //    <tag k="pedestrian" v="no"/>
+  //    <tag k="pedestrian:conditional" v="yes @ (Mo-Sa 08:00-15:00)"/>


I had to tweak the tags manually in the osm-file in order to get the permutations of test-cases I needed.

Conditionals dropped here for clarity.

purew · 2020-01-17T20:34:24Z

valhalla/sif/dynamiccost.h

-                                               cr->end_day_dow(), current_time,
-                                               baldr::DateTime::get_tz_db().from_index(tz_index))) {
+            LOGLN_WARN("Restricted: " + std::to_string(cr->begin_hrs()));
+            // TODO Possibly a bug here. Shouldn't both kTimedDenied and kTimedAllowed


There may be a latent bug here. I haven't dug too closely but I would assume we should be handling both cases as is done in IsRestricted here.

purew · 2020-01-17T20:34:53Z

valhalla/sif/dynamiccost.h

-                                               cr->begin_day_dow(), cr->end_week(), cr->end_month(),
-                                               cr->end_day_dow(), current_time,
-                                               baldr::DateTime::get_tz_db().from_index(tz_index))) {
+            LOGLN_WARN("Restricted: " + std::to_string(cr->begin_hrs()));


TODO Remove stray debug log line.

purew · 2020-01-17T22:27:53Z

Hmm there is some race condition in the tile building stage that occurs in CI but almost never locally...

kevinkreiser · 2020-01-18T00:29:36Z

src/thor/timedep_forward.cc

@@ -148,8 +148,6 @@ inline bool TimeDepForward::ExpandForwardInner(GraphReader& graphreader,
                                               const valhalla::Location& destination,
                                               std::pair<int32_t, float>& best_path) {

-  LOG_WARN(std::string("Expanding edge_id ") + std::to_string(meta.edge_id.id()) + " from node " +
-           std::to_string(pred.endnode().id()));


oh man! that was unfortunate!

purew · 2020-01-22T22:26:23Z

I removed the changes to the scripts/valhalla_build_timezones and now the circleci osx tests pass. Other osx environments seem to not exhibit the same bug.

danpaz · 2020-01-22T22:35:10Z

test/CMakeLists.txt

      ${VALHALLA_SOURCE_DIR}/test/data/utrecht_netherlands.osm.pbf
  COMMAND ${CMAKE_BINARY_DIR}/valhalla_add_predicted_traffic
-      --inline-config '{"mjolnir":{"tile_dir":"test/data/utrecht_tiles","concurrency":4,"logging":{"type":""}}}'
+      --inline-config '{"mjolnir":{"tile_dir":"test/data/utrecht_tiles","concurrency":1,"logging":{"type":""}}}'


Was setting the concurrency to 1 needed to get the tests to build?

Hmm This was a leftover from trying out all the various things.

That said, we are now building 5 different tile-sets on a full test-run and there are more pbf's being added in a different PR. 5 tilesets with 4 threads each results in 20 running threads.

It may just be best to do this single threaded.

danpaz · 2020-01-22T22:38:26Z

test/astar.cc

+
+void test_route_allowed(std::string costing_str, int16_t hour) {
+  auto response = route_on_timerestricted(costing_str, hour);
+  const auto& legs = response.trip().routes(0).legs();


Looks like legs is unused here. Did you plan to add an assertion here like legs.size() == 1 ?

Isn't this throwing a warning for unused variable? Or maybe it is and its getting lost in the noise?

Good catch! 👍

Pushed new code to test for legs.size() == 1

Isn't this throwing a warning for unused variable? Or maybe it is and its getting lost in the noise?

We're not yet treating all warnings as errors unfortunately.

The time-restriction parsing had a logic bug that meant permission was allowed at restricted times and vice versa. This commit also adds a unit-test to verify this functionality.

to only do the generation work once

danpaz

👍

purew requested review from kevinkreiser, gknisely, kdiluca and mandeepsandhu January 16, 2020 23:02

purew force-pushed the test-time-restrictions branch 2 times, most recently from 9a4ad47 to 54641da Compare January 16, 2020 23:16

gknisely previously approved these changes Jan 16, 2020

View reviewed changes

mandeepsandhu reviewed Jan 17, 2020

View reviewed changes

purew dismissed gknisely’s stale review via edd1398 January 17, 2020 13:33

purew force-pushed the test-time-restrictions branch from 54641da to edd1398 Compare January 17, 2020 13:33

kdiluca previously approved these changes Jan 17, 2020

View reviewed changes

purew force-pushed the test-time-restrictions branch from edd1398 to f5a851a Compare January 17, 2020 20:30

purew commented Jan 17, 2020

View reviewed changes

purew force-pushed the test-time-restrictions branch from f5a851a to f01cd63 Compare January 17, 2020 20:42

gknisely previously approved these changes Jan 17, 2020

View reviewed changes

purew dismissed gknisely’s stale review via e8f7f06 January 17, 2020 22:12

kevinkreiser reviewed Jan 18, 2020

View reviewed changes

purew force-pushed the test-time-restrictions branch 2 times, most recently from bdeb106 to 1c42234 Compare January 22, 2020 21:47

danpaz reviewed Jan 22, 2020

View reviewed changes

Fixes bug with inverted time-restriction parsing

006d557

The time-restriction parsing had a logic bug that meant permission was allowed at restricted times and vice versa. This commit also adds a unit-test to verify this functionality.

Breaks out tz-generation to single step

353e82d

to only do the generation work once

purew force-pushed the test-time-restrictions branch from 1c42234 to 353e82d Compare January 22, 2020 23:08

danpaz approved these changes Jan 23, 2020

View reviewed changes

purew merged commit d2e656b into master Jan 23, 2020

purew mentioned this pull request Jan 23, 2020

Removes leftover debug statement #2174

Closed

purew deleted the test-time-restrictions branch February 13, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes bug with inverted time-restriction parsing #2167

Fixes bug with inverted time-restriction parsing #2167

purew commented Jan 16, 2020

mandeepsandhu Jan 17, 2020

purew Jan 17, 2020

purew commented Jan 17, 2020 •

edited

Loading

kdiluca commented Jan 17, 2020

purew Jan 17, 2020

purew Jan 17, 2020

purew Jan 17, 2020

purew Jan 17, 2020 •

edited

Loading

purew Jan 17, 2020

purew commented Jan 17, 2020

kevinkreiser Jan 18, 2020

purew commented Jan 22, 2020

danpaz Jan 22, 2020

purew Jan 22, 2020

danpaz Jan 22, 2020

mandeepsandhu Jan 22, 2020

purew Jan 22, 2020

purew Jan 22, 2020

purew Jan 23, 2020 •

edited

Loading

danpaz left a comment

Fixes bug with inverted time-restriction parsing #2167

Fixes bug with inverted time-restriction parsing #2167

Conversation

purew commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purew commented Jan 17, 2020 • edited Loading

kdiluca commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purew Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purew commented Jan 17, 2020

Choose a reason for hiding this comment

purew commented Jan 22, 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

purew Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

danpaz left a comment

Choose a reason for hiding this comment

purew commented Jan 17, 2020 •

edited

Loading

purew Jan 17, 2020 •

edited

Loading

purew Jan 23, 2020 •

edited

Loading