From 85994c23b3797b10e30c198c07297a01985fe239 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Mon, 20 Nov 2023 22:54:23 +0100 Subject: [PATCH] fix: SearchWindow for arriveBy search is of by one In the current version the dynamic search-window is set 1 minute too early to include a trip if both the minimum travel time can be calculated exact, and the journey arrival-time is withing 59s of the LAT. --- .../transit/RaptorSearchWindowCalculator.java | 9 ++- .../RaptorSearchWindowCalculatorTest.java | 71 +++++++++++++++---- 2 files changed, 62 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculator.java b/src/main/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculator.java index 31608622bc5..bbb5a25c11e 100644 --- a/src/main/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculator.java +++ b/src/main/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculator.java @@ -82,7 +82,7 @@ public RaptorSearchWindowCalculator calculate() { } // TravelWindow is the time from the earliest-departure-time to the latest-arrival-time - int travelWindow = searchWindowSeconds + roundUpToNearestMinute(heuristicMinTransitTime); + int travelWindow = searchWindowSeconds + roundDownToNearestMinute(heuristicMinTransitTime); if (!params.isEarliestDepartureTimeSet()) { earliestDepartureTime = latestArrivalTime - travelWindow; @@ -90,15 +90,14 @@ public RaptorSearchWindowCalculator calculate() { return this; } - int roundUpToNearestMinute(int minTravelTimeInSeconds) { + int roundDownToNearestMinute(int minTravelTimeInSeconds) { if (minTravelTimeInSeconds < 0) { throw new IllegalArgumentException( "This operation is not defined for negative numbers: " + minTravelTimeInSeconds ); } - // See the UnitTest for verification of this: - // We want: 0 -> 0, 1 -> 60, 59 -> 60 ... - return ((minTravelTimeInSeconds + 59) / 60) * 60; + // We want: 0 -> 0, 59 -> 0, 60 -> 60 ... + return (minTravelTimeInSeconds / 60) * 60; } /** diff --git a/src/test/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculatorTest.java b/src/test/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculatorTest.java index 4605d4bfc28..4898c5e2ae9 100644 --- a/src/test/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculatorTest.java +++ b/src/test/java/org/opentripplanner/raptor/rangeraptor/transit/RaptorSearchWindowCalculatorTest.java @@ -2,9 +2,13 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Duration; +import java.util.List; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import org.opentripplanner.raptor._data.transit.TestTripSchedule; import org.opentripplanner.raptor.api.model.RaptorConstants; import org.opentripplanner.raptor.api.request.DynamicSearchWindowCoefficients; @@ -58,19 +62,48 @@ public void calcEarliestDeparture() { subject.withHeuristics(minTransitTime, minWaitTime).withSearchParams(searchParams).calculate(); /* - search-window: round_N(C + T * minTransitTime + W * minWaitTime) + search-window: round_60(C + T * minTransitTime + W * minWaitTime) = round_60(600 + 0.6 * 500 + 0.4 * 200) = round_60(980) = 960 EDT = LAT - (search-window + minTripTime) - EDT = 3000 - (960s + roundUp_60(500)) - EDT = 3000 - (960s + 540s) - EDT = 1500 + EDT = 3000 - (960s + round_60(500)) + EDT = 3000 - (960s + 480s) + EDT = 1560 */ assertEquals(960, subject.getSearchWindowSeconds()); - assertEquals(1_500, subject.getEarliestDepartureTime()); + assertEquals(1_560, subject.getEarliestDepartureTime()); // Given - verify not changed assertEquals(3_000, subject.getLatestArrivalTime()); + assertTrue( + minTransitTime > + subject.getLatestArrivalTime() - + (subject.getSearchWindowSeconds() + subject.getEarliestDepartureTime()) + ); + } + + @Test + public void calcEarliestDepartureExact() { + SearchParams searchParams = new RaptorRequestBuilder() + .searchParams() + .latestArrivalTime(3_000) + .buildSearchParam(); + + int minTransitTime = 600; + int minWaitTime = 0; + + subject.withHeuristics(minTransitTime, minWaitTime).withSearchParams(searchParams).calculate(); + + assertEquals(960, subject.getSearchWindowSeconds()); + assertEquals(1_440, subject.getEarliestDepartureTime()); + // Given - verify not changed + assertEquals(3_000, subject.getLatestArrivalTime()); + + assertEquals( + minTransitTime, + subject.getLatestArrivalTime() - + (subject.getSearchWindowSeconds() + subject.getEarliestDepartureTime()) + ); } @Test @@ -166,17 +199,23 @@ public void calculateNotDefinedIfMinTravelTimeNotSet() { assertThrows(IllegalArgumentException.class, subject::calculate); } - @Test - public void roundUpToNearestMinute() { - assertEquals(0, subject.roundUpToNearestMinute(0)); - assertEquals(60, subject.roundUpToNearestMinute(1)); - assertEquals(60, subject.roundUpToNearestMinute(60)); - assertEquals(120, subject.roundUpToNearestMinute(61)); + static List roundUpToNearestMinuteTestCase() { + return List.of(V2.of(0, 0), V2.of(0, 59), V2.of(60, 60), V2.of(60, 119), V2.of(120, 120)); + } + + @ParameterizedTest + @MethodSource("roundUpToNearestMinuteTestCase") + void roundUpToNearestMinute(V2 v2) { + assertEquals(v2.expected(), subject.roundDownToNearestMinute(v2.value())); } @Test - public void roundUpToNearestMinuteNotDefinedForNegativeNumbers() { - assertThrows(IllegalArgumentException.class, () -> subject.roundUpToNearestMinute(-1)); + void roundUpToNearestMinuteNotDefinedForNegativeNumbers() { + var ex = assertThrows( + IllegalArgumentException.class, + () -> subject.roundDownToNearestMinute(-1) + ); + assertEquals("This operation is not defined for negative numbers: -1", ex.getMessage()); } @Test @@ -188,4 +227,10 @@ public void roundStep() { assertEquals(60, subject.roundStep(30f)); assertEquals(480, subject.roundStep(450f)); } + + record V2(int expected, int value) { + static V2 of(int expected, int value) { + return new V2(expected, value); + } + } }