handling time dependent restrictions #3029

CuriOusQuOkka · 2021-04-23T10:35:58Z

Issue

Valhalla missed conditional TR mapped according to previous tagging schema #441

Tasklist

Add tests
Update the changelog

mandeepsandhu · 2021-04-23T17:19:26Z

test/gurka/test_time_dependent_restrictions.cc

+TEST_F(TimeDependentRestrictions, ActiveHourRestriction) {
+  auto result =
+      gurka::do_action(valhalla::Options::route, map, {"A", "F"}, "auto",
+                       {{"/date_time/type", "1"}, {"/date_time/value", "2021-04-02T14:00"}});


Can we test it for other date_time types (0,2 & 3)? These types invoke different routing algorithms so it'll be good to test the timed TRs here in case it breaks in one algo and not another.

You can use parametrized tests to pass each of the date_time types.

If any of them are failing, they can be flagged with a DISABLED_ prefix - at least then we'll get notice that they're not fixed yet, and we could add a CI step to try non-fatally running disabled tests :

https://google.github.io/googletest/advanced.html#temporarily-disabling-tests

mandeepsandhu · 2021-04-23T17:22:11Z

test/timeparsing.cc

@@ -401,6 +401,29 @@ TEST(TimeParsing, TestConditionalRestrictions) {
      TryConditionalRestrictions(conditions.at(x), 0, 1, 64, 3, 3, 0, 15, 0, 12, 1, 5, 17, 0);
    }
  }
+


Do we want to test wrongly formatted restriction...eg: end day is set without a corresponding start day (-Friday or some such)?

As for me it sounds as incorrect input. In case only day_on is specified, '-' is not added, day_on can be a sequence of days separated by a comma or a range with '-' inside. When only day_off is specified it is harder to understand what it means: working hours are from Monday/Sunday to X, or each day except X....?

You're right, in that the input is incorrect. My point was that we should test that such inputs are ignored (looking at the code, we only set the condition when day_start is set, so just having a day_end should not enable the TR). So adding a test with incorrect input and showing that it does not take effect would be sufficient. Does that make sense?

Yeah, added regular and gurka tests.

@CuriOusQuOkka @mandeepsandhu if we have only the start day set then these restrictions should be tossed as the user entered the info incorrectly

genadz · 2021-04-26T12:09:08Z

CHANGELOG.md

@@ -52,6 +52,7 @@
   * FIXED: Removed unexpected maneuvers at sharp bends [#2968](https://github.com/valhalla/valhalla/pull/2968)
   * FIXED: Remove large number formatting for non-US countries [#3015](https://github.com/valhalla/valhalla/pull/3015)
   * FIXED: Odin undefined behaviour: handle case when xedgeuse is not initialized [#3020](https://github.com/valhalla/valhalla/pull/3020)
+   * FIXED: Fix time-dependent restrictions handling [#3029](https://github.com/valhalla/valhalla/pull/3029)


this is too general description, I'd recommend to make it more specific

genadz · 2021-04-26T12:49:06Z

src/mjolnir/pbfgraphparser.cc

+              if (!day_start.empty() && !day_end.empty()) {
+                condition = day_start + '-' + day_end;
+              } else if (!day_start.empty()) {
+                condition = day_start;


so, just to clarify: if we have only day_start it means that restriction applies only on this day ? or to the end of the week ?

@genadz we can't only have just a day_start. if the restriction is only one day the day_start would be equal to the day_end.

@gknisely from the code above it looks like just having day_start is also a valid option. If both start & end are mandatory, then maybe we should just have if (!day_start.empty() && !day_end.empty()) { ...} ?

@mandeepsandhu correct.

We support start date that stays alone. It can be a sequence (like Monday,Wednesday,Thursday), a range (Monday-Friday), or just one day.

read the comment form @gknisely. Ignore this case.

genadz · 2021-04-26T15:31:22Z

test/gurka/test_time_dependent_restrictions.cc

+};
+gurka::map TimeDependentRestrictions::map = {};
+
+// TODO: Does not work for date_time_type=2, returns additional AB edge.


it's because of this #2940 ; try to put start and end locations on the edges

D----B---1-A | | | | E----C---2-F

@genadz since date_time: 2 uses timedep-reverse, I wonder if the extra edge is being returned due to the bug @danpat found here.

@CuriOusQuOkka #2987 fixed a bug in timedep-reverse, which might fix the problem you're seeing. If so, you can re-enable date_time: 2.

Thanks @mandeepsandhu, it fixed the bug.

@genadz since date_time: 2 uses timedep-reverse, I wonder if the extra edge is being returned due to the bug @danpat found here.

cool! so, I should close this issue #2940

mandeepsandhu · 2021-04-26T19:34:57Z

@CuriOusQuOkka do you want to remove the "Draft" tag from this PR (thats a signal to reviewers to start looking at the changes more closely)?

genadz · 2021-04-27T07:24:50Z

test/gurka/test_time_dependent_tags.cc

+}
+
+TEST_F(TimeDependentTags, IgnoreDayOffOnlyRestriction) {
+  tags day_off = {


if I understood correctly having only day_on or hour_off/hour_on also should be ignored. could you add all these cases ?

gknisely · 2021-04-27T12:22:29Z

@CuriOusQuOkka I think we will be good after we test with https://github.com/valhalla/valhalla/blob/master/test_requests/timed_access_routes.txt and https://github.com/valhalla/valhalla/blob/master/test_requests/timed_conditional_routes.txt Let me know if there are any diffs.

mandeepsandhu · 2021-04-28T20:20:34Z

test/gurka/test_time_dependent_restrictions.cc

+                                  {"/date_time/value", "2021-04-02T23:00"}});
+  gurka::assert::raw::expect_path(result, {"AB", "BC", "CF"});
+}
+


For completeness, should we also add a testcase with date_time before start of restriction?

CuriOusQuOkka · 2021-04-29T11:11:14Z

@CuriOusQuOkka I think we will be good after we test with https://github.com/valhalla/valhalla/blob/master/test_requests/timed_access_routes.txt and https://github.com/valhalla/valhalla/blob/master/test_requests/timed_conditional_routes.txt Let me know if there are any diffs.

Rad tests have passed

CuriOusQuOkka requested a review from genadz April 23, 2021 10:35

CuriOusQuOkka changed the title ~~handling old-style time dependent restrictions~~ handling time dependent restrictions Apr 23, 2021

CuriOusQuOkka force-pushed the time_dependent_restrictions branch from 7deec93 to 771669a Compare April 23, 2021 10:39

mandeepsandhu reviewed Apr 23, 2021

View reviewed changes

CuriOusQuOkka force-pushed the time_dependent_restrictions branch from 771669a to 310feb8 Compare April 23, 2021 23:04

genadz requested changes Apr 26, 2021

View reviewed changes

CuriOusQuOkka force-pushed the time_dependent_restrictions branch 2 times, most recently from 85c6ad1 to cc3af61 Compare April 26, 2021 17:48

CuriOusQuOkka force-pushed the time_dependent_restrictions branch 3 times, most recently from 0b9c86d to 7a95f68 Compare April 26, 2021 21:09

CuriOusQuOkka marked this pull request as ready for review April 26, 2021 21:14

genadz reviewed Apr 27, 2021

View reviewed changes

CuriOusQuOkka force-pushed the time_dependent_restrictions branch 2 times, most recently from dd6690d to e0aec94 Compare April 27, 2021 10:54

CuriOusQuOkka mentioned this pull request Apr 27, 2021

Evaluate restrictions with dow only #3039

Merged

3 tasks

mandeepsandhu reviewed Apr 28, 2021

View reviewed changes

CuriOusQuOkka force-pushed the time_dependent_restrictions branch 2 times, most recently from 5862a44 to 6a95605 Compare April 29, 2021 11:08

handling old-style time dependent restrictions

d435a0b

CuriOusQuOkka force-pushed the time_dependent_restrictions branch from 6a95605 to d435a0b Compare April 29, 2021 11:20

CuriOusQuOkka requested a review from genadz April 29, 2021 11:25

genadz approved these changes Apr 29, 2021

View reviewed changes

CuriOusQuOkka merged commit 01ca643 into valhalla:master Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handling time dependent restrictions #3029

handling time dependent restrictions #3029

CuriOusQuOkka commented Apr 23, 2021 •

edited

Loading

mandeepsandhu Apr 23, 2021

danpat Apr 23, 2021

mandeepsandhu Apr 23, 2021

CuriOusQuOkka Apr 23, 2021

mandeepsandhu Apr 23, 2021

CuriOusQuOkka Apr 23, 2021

gknisely Apr 26, 2021

genadz Apr 26, 2021

genadz Apr 26, 2021

gknisely Apr 26, 2021

mandeepsandhu Apr 26, 2021

gknisely Apr 26, 2021

CuriOusQuOkka Apr 26, 2021

CuriOusQuOkka Apr 26, 2021

genadz Apr 26, 2021

mandeepsandhu Apr 26, 2021

CuriOusQuOkka Apr 26, 2021

genadz Apr 27, 2021

mandeepsandhu commented Apr 26, 2021

genadz Apr 27, 2021

gknisely commented Apr 27, 2021

mandeepsandhu Apr 28, 2021

CuriOusQuOkka commented Apr 29, 2021

handling time dependent restrictions #3029

handling time dependent restrictions #3029

Conversation

CuriOusQuOkka commented Apr 23, 2021 • 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

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

mandeepsandhu commented Apr 26, 2021

Choose a reason for hiding this comment

gknisely commented Apr 27, 2021

Choose a reason for hiding this comment

CuriOusQuOkka commented Apr 29, 2021

CuriOusQuOkka commented Apr 23, 2021 •

edited

Loading