From 5b6cfdad3b74bc35e7c57f84814426f398123573 Mon Sep 17 00:00:00 2001 From: Henrik Abrahamsson Date: Wed, 3 Apr 2024 14:06:47 +0200 Subject: [PATCH 1/2] Fix bug in heuristics cost calculation for egress legs --- .../heuristics/HeuristicsAdapter.java | 8 +- .../raptor/spi/RaptorCostCalculator.java | 6 +- .../transit/cost/CostCalculatorFactory.java | 4 +- .../transit/cost/DefaultCostCalculator.java | 23 ++++-- .../transit/cost/PatternCostCalculator.java | 4 +- .../cost/WheelchairCostCalculator.java | 4 +- .../raptor/_data/RaptorTestConstants.java | 2 + .../raptor/_data/transit/TestTransitData.java | 9 ++- .../B05_EgressStopTransferCostTest.java | 76 +++++++++++++++++++ .../cost/DefaultCostCalculatorTest.java | 18 +++-- 10 files changed, 129 insertions(+), 25 deletions(-) create mode 100644 src/test/java/org/opentripplanner/raptor/moduletests/B05_EgressStopTransferCostTest.java diff --git a/src/main/java/org/opentripplanner/raptor/rangeraptor/standard/heuristics/HeuristicsAdapter.java b/src/main/java/org/opentripplanner/raptor/rangeraptor/standard/heuristics/HeuristicsAdapter.java index 107d41208f4..3dbff516347 100644 --- a/src/main/java/org/opentripplanner/raptor/rangeraptor/standard/heuristics/HeuristicsAdapter.java +++ b/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 + ) + ); } /** diff --git a/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java b/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java index d575a0e4d8c..f69d5410fc4 100644 --- a/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java +++ b/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 + * 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} retuning * the greatest value, which is guaranteed to be less than the - * real value would be correct and a good choose. + * real value 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 diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java index fef7b3509e3..3a79b25e678 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java @@ -6,11 +6,11 @@ public class CostCalculatorFactory { public static RaptorCostCalculator createCostCalculator( GeneralizedCostParameters generalizedCostParameters, - int[] stopBoardAlightCosts + int[] stopTransferCosts ) { RaptorCostCalculator calculator = new DefaultCostCalculator<>( generalizedCostParameters, - stopBoardAlightCosts + stopTransferCosts ); if (generalizedCostParameters.wheelchairEnabled()) { diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java index 26a1286d10d..d5951812612 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java +++ b/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 if (stopTransferCost != null) { + // Remove cost that was added during alighting similar as we do in the costEgress() method + return (transitFactors.minFactor() * minTravelTime - stopTransferCost[fromStop]); + } else { + return transitFactors.minFactor() * minTravelTime; + } } @Override diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/PatternCostCalculator.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/PatternCostCalculator.java index ecd0704bbc2..734355f0ac5 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/PatternCostCalculator.java +++ b/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 diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/WheelchairCostCalculator.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/WheelchairCostCalculator.java index 78014bd245d..d0697f78692 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/WheelchairCostCalculator.java +++ b/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 diff --git a/src/test/java/org/opentripplanner/raptor/_data/RaptorTestConstants.java b/src/test/java/org/opentripplanner/raptor/_data/RaptorTestConstants.java index 0bca83be8bf..78489508dc4 100644 --- a/src/test/java/org/opentripplanner/raptor/_data/RaptorTestConstants.java +++ b/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; diff --git a/src/test/java/org/opentripplanner/raptor/_data/transit/TestTransitData.java b/src/test/java/org/opentripplanner/raptor/_data/transit/TestTransitData.java index 2a77996b47b..037c345bc23 100644 --- a/src/test/java/org/opentripplanner/raptor/_data/transit/TestTransitData.java +++ b/src/test/java/org/opentripplanner/raptor/_data/transit/TestTransitData.java @@ -66,6 +66,8 @@ public class TestTransitData private final List 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 getPatterns() { private int[] stopBoardAlightCost() { // Not implemented, no test for this yet. - return null; + return stopBoardAlightCosts; } private void expandNumOfStops(int stopIndex) { diff --git a/src/test/java/org/opentripplanner/raptor/moduletests/B05_EgressStopTransferCostTest.java b/src/test/java/org/opentripplanner/raptor/moduletests/B05_EgressStopTransferCostTest.java new file mode 100644 index 00000000000..21c1dd2a755 --- /dev/null +++ b/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 + *

+ * 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 requestBuilder = new RaptorRequestBuilder<>(); + private final RaptorService 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 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)); + } +} diff --git a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculatorTest.java b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculatorTest.java index 7b7ba23499c..3570d92bdbe 100644 --- a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculatorTest.java +++ b/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) ); } From a5d91fb0e8cf12a8ba38dd76ed257e2b4ab732d1 Mon Sep 17 00:00:00 2001 From: Henrik Abrahamsson Date: Thu, 25 Apr 2024 14:59:04 +0200 Subject: [PATCH 2/2] Resolve review comments --- .../opentripplanner/raptor/spi/RaptorCostCalculator.java | 2 +- .../raptoradapter/transit/cost/CostCalculatorFactory.java | 4 ++-- .../raptoradapter/transit/cost/DefaultCostCalculator.java | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java b/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java index f69d5410fc4..7a9928e51aa 100644 --- a/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java +++ b/src/main/java/org/opentripplanner/raptor/spi/RaptorCostCalculator.java @@ -53,7 +53,7 @@ int boardingCost( /** * Used for estimating the remaining value for a criteria at a given stop arrival. The calculated * 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} retuning + * the generalized cost for given the {@code minTravelTime} and {@code minNumTransfers} returning * the greatest value, which is guaranteed to be less than the * real value would be correct and a good choice. */ diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java index 3a79b25e678..fef7b3509e3 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/CostCalculatorFactory.java @@ -6,11 +6,11 @@ public class CostCalculatorFactory { public static RaptorCostCalculator createCostCalculator( GeneralizedCostParameters generalizedCostParameters, - int[] stopTransferCosts + int[] stopBoardAlightCosts ) { RaptorCostCalculator calculator = new DefaultCostCalculator<>( generalizedCostParameters, - stopTransferCosts + stopBoardAlightCosts ); if (generalizedCostParameters.wheelchairEnabled()) { diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java index d5951812612..ee2f1282625 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/cost/DefaultCostCalculator.java @@ -130,11 +130,11 @@ public int calculateRemainingMinCost(int minTravelTime, int minNumTransfers, int transitFactors.minFactor() * minTravelTime ); - } else if (stopTransferCost != null) { - // Remove cost that was added during alighting similar as we do in the costEgress() method - return (transitFactors.minFactor() * minTravelTime - stopTransferCost[fromStop]); } else { - return transitFactors.minFactor() * minTravelTime; + // 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]; } }