Merge pull request opentripplanner#5783 from Skanetrafiken/fix-egress-transfer-cost-bug

Fix bug in heuristics cost calculation for egress legs

Author: habrahamsson-skanetrafiken

src/main/java/org/opentripplanner/raptor/rangeraptor/standard/heuristics/HeuristicsAdapter.java

@@ -140,7 +140,13 @@ private int bestNumOfTransfers(int stop) {
   }
 
   private int bestGeneralizedCost(int stop) {
-    return costCalculator.calculateMinCost(bestTravelDuration(stop), bestNumOfTransfers(stop));
+    return (
+      costCalculator.calculateRemainingMinCost(
+        bestTravelDuration(stop),
+        bestNumOfTransfers(stop),
+        stop
+      )
+    );
   }
 
   /**

src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java

@@ -52,12 +52,12 @@ int boardingCost(
 
   /**
    * Used for estimating the remaining value for a criteria at a given stop arrival. The calculated
-   * value should be a an optimistic estimate for the heuristics to work properly. So, to calculate
-   * the generalized cost for given the {@code minTravelTime} and {@code minNumTransfers} retuning
+   * value should be an optimistic estimate for the heuristics to work properly. So, to calculate
+   * the generalized cost for given the {@code minTravelTime} and {@code minNumTransfers} returning
    * the greatest value, which is guaranteed to be less than the
-   * <em>real value</em> would be correct and a good choose.
+   * <em>real value</em> would be correct and a good choice.
    */
-  int calculateMinCost(int minTravelTime, int minNumTransfers);
+  int calculateRemainingMinCost(int minTravelTime, int minNumTransfers, int fromStop);
 
   /**
    * This method allows the cost calculator to add cost in addition to the generalized-cost of the

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java

@@ -121,14 +121,21 @@ public int waitCost(int waitTimeInSeconds) {
   }
 
   @Override
-  public int calculateMinCost(int minTravelTime, int minNumTransfers) {
-    return (
-      boardCostOnly +
-      boardAndTransferCost *
-      minNumTransfers +
-      transitFactors.minFactor() *
-      minTravelTime
-    );
+  public int calculateRemainingMinCost(int minTravelTime, int minNumTransfers, int fromStop) {
+    if (minNumTransfers > -1) {
+      return (
+        boardCostOnly +
+        boardAndTransferCost *
+        minNumTransfers +
+        transitFactors.minFactor() *
+        minTravelTime
+      );
+    } else {
+      // Remove cost that was added during alighting similar as we do in the costEgress() method
+      int fixedCost = transitFactors.minFactor() * minTravelTime;
+
+      return stopTransferCost == null ? fixedCost : fixedCost - stopTransferCost[fromStop];
+    }
   }
 
   @Override

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/PatternCostCalculator.java

@@ -77,8 +77,8 @@ public int waitCost(int waitTimeInSeconds) {
   }
 
   @Override
-  public int calculateMinCost(int minTravelTime, int minNumTransfers) {
-    return delegate.calculateMinCost(minTravelTime, minNumTransfers);
+  public int calculateRemainingMinCost(int minTravelTime, int minNumTransfers, int fromStop) {
+    return delegate.calculateRemainingMinCost(minTravelTime, minNumTransfers, fromStop);
   }
 
   @Override

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/WheelchairCostCalculator.java

@@ -66,8 +66,8 @@ public int waitCost(int waitTimeInSeconds) {
   }
 
   @Override
-  public int calculateMinCost(int minTravelTime, int minNumTransfers) {
-    return delegate.calculateMinCost(minTravelTime, minNumTransfers);
+  public int calculateRemainingMinCost(int minTravelTime, int minNumTransfers, int fromStop) {
+    return delegate.calculateRemainingMinCost(minTravelTime, minNumTransfers, fromStop);
   }
 
   @Override

src/test/java/org/opentripplanner/raptor/_data/RaptorTestConstants.java

@@ -60,6 +60,8 @@ public interface RaptorTestConstants {
   int STOP_L = 12;
   int STOP_M = 13;
 
+  int NUM_STOPS = 14;
+
   // Stop position in pattern
   int STOP_POS_0 = 0;
   int STOP_POS_1 = 1;

src/test/java/org/opentripplanner/raptor/_data/transit/TestTransitData.java

@@ -66,6 +66,8 @@ public class TestTransitData
   private final List<ConstrainedTransfer> constrainedTransfers = new ArrayList<>();
   private final GeneralizedCostParametersBuilder costParamsBuilder = GeneralizedCostParameters.of();
 
+  private final int[] stopBoardAlightCosts = new int[NUM_STOPS];
+
   private RaptorSlackProvider slackProvider = SLACK_PROVIDER;
 
   @Override
@@ -300,6 +302,11 @@ public TestTransitData withConstrainedTransfer(
     return this;
   }
 
+  public TestTransitData withStopBoardAlightCost(int stop, int boardAlightCost) {
+    stopBoardAlightCosts[stop] = boardAlightCost;
+    return this;
+  }
+
   public GeneralizedCostParametersBuilder mcCostParamsBuilder() {
     return costParamsBuilder;
   }
@@ -352,7 +359,7 @@ public List<DefaultTripPattern> getPatterns() {
 
   private int[] stopBoardAlightCost() {
     // Not implemented, no test for this yet.
-    return null;
+    return stopBoardAlightCosts;
   }
 
   private void expandNumOfStops(int stopIndex) {

src/test/java/org/opentripplanner/raptor/moduletests/B05_EgressStopTransferCostTest.java

@@ -0,0 +1,76 @@
+package org.opentripplanner.raptor.moduletests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.opentripplanner.raptor._data.transit.TestRoute.route;
+import static org.opentripplanner.raptor._data.transit.TestTripSchedule.schedule;
+import static org.opentripplanner.raptor.moduletests.support.RaptorModuleTestConfig.multiCriteria;
+
+import java.util.List;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.opentripplanner.raptor.RaptorService;
+import org.opentripplanner.raptor._data.RaptorTestConstants;
+import org.opentripplanner.raptor._data.transit.TestAccessEgress;
+import org.opentripplanner.raptor._data.transit.TestTransitData;
+import org.opentripplanner.raptor._data.transit.TestTripSchedule;
+import org.opentripplanner.raptor.api.request.RaptorRequestBuilder;
+import org.opentripplanner.raptor.configure.RaptorConfig;
+import org.opentripplanner.raptor.moduletests.support.ModuleTestDebugLogging;
+import org.opentripplanner.raptor.moduletests.support.RaptorModuleTestCase;
+
+/**
+ * FEATURE UNDER TEST
+ * <p>
+ * This verifies that the stopTransferCost is not applied for egress legs. If this is not correctly
+ * handled by the heuristics optimization, the cheapest journey could be discarded.
+ */
+public class B05_EgressStopTransferCostTest implements RaptorTestConstants {
+
+  private final TestTransitData data = new TestTransitData();
+  private final RaptorRequestBuilder<TestTripSchedule> requestBuilder = new RaptorRequestBuilder<>();
+  private final RaptorService<TestTripSchedule> raptorService = new RaptorService<>(
+    RaptorConfig.defaultConfigForTest()
+  );
+
+  @BeforeEach
+  void setup() {
+    data
+      .withRoute(route("R1", STOP_B, STOP_C).withTimetable(schedule("0:10, 0:14")))
+      .withRoute(route("R2", STOP_C, STOP_D).withTimetable(schedule("0:18, 0:20")));
+
+    data.mcCostParamsBuilder().transferCost(0).boardCost(0);
+    data.withStopBoardAlightCost(STOP_D, 60000);
+
+    requestBuilder
+      .searchParams()
+      .addAccessPaths(TestAccessEgress.free(STOP_B))
+      .addEgressPaths(
+        TestAccessEgress.walk(STOP_C, D5m), // This will be the fastest
+        TestAccessEgress.walk(STOP_D, D20s) // This will be the cheapest
+      )
+      .earliestDepartureTime(T00_00)
+      .latestArrivalTime(T00_30);
+
+    ModuleTestDebugLogging.setupDebugLogging(data, requestBuilder);
+  }
+
+  static List<RaptorModuleTestCase> testCases() {
+    return RaptorModuleTestCase
+      .of()
+      .add(
+        multiCriteria(),
+        // We should get both the fastest and the c1-cheapest results
+        // The stopTransferCost should not be applied to the egress leg from STOP_D
+        "B ~ BUS R1 0:10 0:14 ~ C ~ Walk 5m [0:10 0:19 9m Tₓ0 C₁840]",
+        "B ~ BUS R1 0:10 0:14 ~ C ~ BUS R2 0:18 0:20 ~ D ~ Walk 20s [0:10 0:20:20 10m20s Tₓ1 C₁640]"
+      )
+      .build();
+  }
+
+  @ParameterizedTest
+  @MethodSource("testCases")
+  void testRaptor(RaptorModuleTestCase testCase) {
+    assertEquals(testCase.expected(), testCase.run(raptorService, data, requestBuilder));
+  }
+}

src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculatorTest.java

@@ -76,22 +76,28 @@ public void calculateMinCost() {
     //   - Transit factor:  80 (min of 80 and 100)
 
     // Board cost is 500:
-    assertEquals(500, subject.calculateMinCost(0, 0));
+    assertEquals(500, subject.calculateRemainingMinCost(0, 0, 0));
     // The transfer 1s * 80 = 80 + board cost 500
-    assertEquals(580, subject.calculateMinCost(1, 0));
+    assertEquals(580, subject.calculateRemainingMinCost(1, 0, 0));
     // Board 2 times and transfer 1: 2 * 500 + 200
-    assertEquals(1200, subject.calculateMinCost(0, 1));
+    assertEquals(1200, subject.calculateRemainingMinCost(0, 1, 0));
 
     // Transit 200s * 80 + Board 4 * 500 + Transfer 3 * 200
-    assertEquals(18_600, subject.calculateMinCost(200, 3));
+    assertEquals(18_600, subject.calculateRemainingMinCost(200, 3, 0));
+
+    // Cost of egress should subtract the stop transfer cost 25
+    assertEquals(-25, subject.calculateRemainingMinCost(0, -1, 1));
   }
 
   @Test
   public void testConvertBetweenRaptorAndMainOtpDomainModel() {
-    assertEquals(RaptorCostConverter.toRaptorCost(BOARD_COST_SEC), subject.calculateMinCost(0, 0));
+    assertEquals(
+      RaptorCostConverter.toRaptorCost(BOARD_COST_SEC),
+      subject.calculateRemainingMinCost(0, 0, 0)
+    );
     assertEquals(
       RaptorCostConverter.toRaptorCost(0.8 * 20 + BOARD_COST_SEC),
-      subject.calculateMinCost(20, 0)
+      subject.calculateRemainingMinCost(20, 0, 0)
     );
   }