From 94403bffcb03f398bf2fe6524620beb294b8827c Mon Sep 17 00:00:00 2001 From: Henrik Abrahamsson Date: Thu, 26 Sep 2024 16:39:24 +0200 Subject: [PATCH] Fix max-stop limit in StreetNearbyStopFinder --- .../strategy/MaxCountSkipEdgeStrategy.java | 34 --------- .../strategy/MaxCountTerminationStrategy.java | 36 ++++++++++ .../nearbystops/StreetNearbyStopFinder.java | 41 +++++------ .../MaxCountSkipEdgeStrategyTest.java | 38 ---------- .../MaxCountTerminationStrategyTest.java | 29 ++++++++ ...reetNearbyStopFinderMultipleLinksTest.java | 71 +++++++++++++++++++ .../StreetNearbyStopFinderTest.java | 8 +-- 7 files changed, 156 insertions(+), 101 deletions(-) delete mode 100644 application/src/main/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategy.java create mode 100644 application/src/main/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategy.java delete mode 100644 application/src/test/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategyTest.java create mode 100644 application/src/test/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategyTest.java create mode 100644 application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderMultipleLinksTest.java diff --git a/application/src/main/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategy.java b/application/src/main/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategy.java deleted file mode 100644 index 0369e3e29db..00000000000 --- a/application/src/main/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategy.java +++ /dev/null @@ -1,34 +0,0 @@ -package org.opentripplanner.astar.strategy; - -import java.util.function.Predicate; -import org.opentripplanner.astar.spi.AStarEdge; -import org.opentripplanner.astar.spi.AStarState; -import org.opentripplanner.astar.spi.SkipEdgeStrategy; - -/** - * Skips edges when the specified number of desired vertices have been visited. - */ -public class MaxCountSkipEdgeStrategy< - State extends AStarState, Edge extends AStarEdge -> - implements SkipEdgeStrategy { - - private final int maxCount; - private final Predicate shouldIncreaseCount; - - private int visited; - - public MaxCountSkipEdgeStrategy(int count, Predicate shouldIncreaseCount) { - this.maxCount = count; - this.shouldIncreaseCount = shouldIncreaseCount; - this.visited = 0; - } - - @Override - public boolean shouldSkipEdge(State current, Edge edge) { - if (shouldIncreaseCount.test(current)) { - visited++; - } - return visited > maxCount; - } -} diff --git a/application/src/main/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategy.java b/application/src/main/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategy.java new file mode 100644 index 00000000000..66c5496c923 --- /dev/null +++ b/application/src/main/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategy.java @@ -0,0 +1,36 @@ +package org.opentripplanner.astar.strategy; + +import java.util.function.Predicate; +import org.opentripplanner.astar.spi.AStarState; +import org.opentripplanner.astar.spi.SearchTerminationStrategy; + +/** + * This termination strategy is used to terminate an a-star search after a number of states matching + * some criteria has been found. For example it can be used to limit a search to a maximum number of + * stops. + */ +public class MaxCountTerminationStrategy> + implements SearchTerminationStrategy { + + private final int maxCount; + private final Predicate shouldIncreaseCount; + private int count; + + /** + * @param maxCount Terminate the search after this many matching states have been reached. + * @param shouldIncreaseCount A predicate to check if a state should increase the count or not. + */ + public MaxCountTerminationStrategy(int maxCount, Predicate shouldIncreaseCount) { + this.maxCount = maxCount; + this.shouldIncreaseCount = shouldIncreaseCount; + this.count = 0; + } + + @Override + public boolean shouldSearchTerminate(State current) { + if (shouldIncreaseCount.test(current)) { + count++; + } + return count >= maxCount; + } +} diff --git a/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java b/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java index e54c27249e1..8277bd47e4c 100644 --- a/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java +++ b/application/src/main/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinder.java @@ -11,10 +11,8 @@ import java.util.List; import java.util.Set; import org.opentripplanner.astar.model.ShortestPathTree; -import org.opentripplanner.astar.spi.SkipEdgeStrategy; -import org.opentripplanner.astar.strategy.ComposingSkipEdgeStrategy; import org.opentripplanner.astar.strategy.DurationSkipEdgeStrategy; -import org.opentripplanner.astar.strategy.MaxCountSkipEdgeStrategy; +import org.opentripplanner.astar.strategy.MaxCountTerminationStrategy; import org.opentripplanner.ext.dataoverlay.routing.DataOverlayContext; import org.opentripplanner.framework.application.OTPFeature; import org.opentripplanner.framework.application.OTPRequestTimeoutException; @@ -30,8 +28,6 @@ import org.opentripplanner.street.model.vertex.Vertex; import org.opentripplanner.street.search.StreetSearchBuilder; import org.opentripplanner.street.search.TraverseMode; -import org.opentripplanner.street.search.request.StreetSearchRequest; -import org.opentripplanner.street.search.request.StreetSearchRequestMapper; import org.opentripplanner.street.search.state.State; import org.opentripplanner.street.search.strategy.DominanceFunctions; import org.opentripplanner.transit.model.site.AreaStop; @@ -46,8 +42,8 @@ public class StreetNearbyStopFinder implements NearbyStopFinder { /** * Construct a NearbyStopFinder for the given graph and search radius. * - * @param maxStopCount The maximum stops to return. 0 means no limit. Regardless of the maxStopCount - * we will always return all the directly connected stops. + * @param maxStopCount The maximum stops to return. 0 means no limit. Regardless of the + * maxStopCount we will always return all the directly connected stops. */ public StreetNearbyStopFinder( Duration durationLimit, @@ -117,23 +113,30 @@ public Collection findNearbyStops( // Return only the origin vertices if there are no valid street modes if ( streetRequest.mode() == StreetMode.NOT_SET || - (maxStopCount != 0 && stopsFound.size() >= maxStopCount) + (maxStopCount > 0 && stopsFound.size() >= maxStopCount) ) { return stopsFound; } stopsFound = new ArrayList<>(stopsFound); - ShortestPathTree spt = StreetSearchBuilder + var streetSearch = StreetSearchBuilder .of() - .setSkipEdgeStrategy(getSkipEdgeStrategy()) + .setSkipEdgeStrategy(new DurationSkipEdgeStrategy<>(durationLimit)) .setDominanceFunction(new DominanceFunctions.MinimumWeight()) .setRequest(request) .setArriveBy(reverseDirection) .setStreetRequest(streetRequest) .setFrom(reverseDirection ? null : originVertices) .setTo(reverseDirection ? originVertices : null) - .setDataOverlayContext(dataOverlayContext) - .getShortestPathTree(); + .setDataOverlayContext(dataOverlayContext); + + if (maxStopCount > 0) { + streetSearch.setTerminationStrategy( + new MaxCountTerminationStrategy<>(maxStopCount, this::hasReachedStop) + ); + } + + ShortestPathTree spt = streetSearch.getShortestPathTree(); // Only used if OTPFeature.FlexRouting.isOn() Multimap locationsMap = ArrayListMultimap.create(); @@ -186,16 +189,6 @@ public Collection findNearbyStops( return stopsFound; } - private SkipEdgeStrategy getSkipEdgeStrategy() { - var durationSkipEdgeStrategy = new DurationSkipEdgeStrategy(durationLimit); - - if (maxStopCount > 0) { - var strategy = new MaxCountSkipEdgeStrategy<>(maxStopCount, this::hasReachedStop); - return new ComposingSkipEdgeStrategy<>(strategy, durationSkipEdgeStrategy); - } - return durationSkipEdgeStrategy; - } - private boolean canBoardFlex(State state, boolean reverse) { Collection edges = reverse ? state.getVertex().getIncoming() @@ -212,13 +205,13 @@ private boolean canBoardFlex(State state, boolean reverse) { *

* This is important because there can be cases where states that cannot actually board the vehicle * can dominate those that can thereby leading to zero found stops when this predicate is used with - * the {@link MaxCountSkipEdgeStrategy}. + * the {@link MaxCountTerminationStrategy}. *

* An example of this would be an egress/reverse search with a very high walk reluctance where the * states that speculatively rent a vehicle move the walk states down the A* priority queue until * the required number of stops are reached to abort the search, leading to zero egress results. */ - public boolean hasReachedStop(State state) { + private boolean hasReachedStop(State state) { var vertex = state.getVertex(); return ( vertex instanceof TransitStopVertex && state.isFinal() && !ignoreVertices.contains(vertex) diff --git a/application/src/test/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategyTest.java b/application/src/test/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategyTest.java deleted file mode 100644 index c190ff1abac..00000000000 --- a/application/src/test/java/org/opentripplanner/astar/strategy/MaxCountSkipEdgeStrategyTest.java +++ /dev/null @@ -1,38 +0,0 @@ -package org.opentripplanner.astar.strategy; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import org.junit.jupiter.api.Test; -import org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinder; -import org.opentripplanner.street.search.state.TestStateBuilder; - -class MaxCountSkipEdgeStrategyTest { - - private final StreetNearbyStopFinder finder = new StreetNearbyStopFinder(null, 0, null); - - @Test - void countStops() { - var state = TestStateBuilder.ofWalking().stop().build(); - var strategy = new MaxCountSkipEdgeStrategy<>(1, finder::hasReachedStop); - assertFalse(strategy.shouldSkipEdge(state, null)); - assertTrue(strategy.shouldSkipEdge(state, null)); - } - - @Test - void doNotCountStop() { - var state = TestStateBuilder.ofWalking().build(); - var strategy = new MaxCountSkipEdgeStrategy<>(1, finder::hasReachedStop); - assertFalse(strategy.shouldSkipEdge(state, null)); - assertFalse(strategy.shouldSkipEdge(state, null)); - assertFalse(strategy.shouldSkipEdge(state, null)); - } - - @Test - void nonFinalState() { - var state = TestStateBuilder.ofScooterRentalArriveBy().stop().build(); - assertFalse(state.isFinal()); - var strategy = new MaxCountSkipEdgeStrategy<>(1, finder::hasReachedStop); - assertFalse(strategy.shouldSkipEdge(state, null)); - } -} diff --git a/application/src/test/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategyTest.java b/application/src/test/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategyTest.java new file mode 100644 index 00000000000..50a920f1252 --- /dev/null +++ b/application/src/test/java/org/opentripplanner/astar/strategy/MaxCountTerminationStrategyTest.java @@ -0,0 +1,29 @@ +package org.opentripplanner.astar.strategy; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class MaxCountTerminationStrategyTest { + + @Test + void countStates() { + var countAllStatesStrategy = new MaxCountTerminationStrategy<>(3, state -> true); + + assertFalse(countAllStatesStrategy.shouldSearchTerminate(null)); + assertFalse(countAllStatesStrategy.shouldSearchTerminate(null)); + assertTrue(countAllStatesStrategy.shouldSearchTerminate(null)); + assertTrue(countAllStatesStrategy.shouldSearchTerminate(null)); + } + + @Test + void countNoStates() { + var countNoStatesStrategy = new MaxCountTerminationStrategy<>(1, state -> false); + + assertFalse(countNoStatesStrategy.shouldSearchTerminate(null)); + assertFalse(countNoStatesStrategy.shouldSearchTerminate(null)); + assertFalse(countNoStatesStrategy.shouldSearchTerminate(null)); + assertFalse(countNoStatesStrategy.shouldSearchTerminate(null)); + } +} diff --git a/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderMultipleLinksTest.java b/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderMultipleLinksTest.java new file mode 100644 index 00000000000..3466c1bfd8a --- /dev/null +++ b/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderMultipleLinksTest.java @@ -0,0 +1,71 @@ +package org.opentripplanner.graph_builder.module.nearbystops; + +import static com.google.common.truth.Truth.assertThat; +import static org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinderTest.assertStopAtDistance; +import static org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinderTest.assertZeroDistanceStop; +import static org.opentripplanner.graph_builder.module.nearbystops.StreetNearbyStopFinderTest.sort; + +import java.time.Duration; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.opentripplanner.framework.geometry.WgsCoordinate; +import org.opentripplanner.routing.algorithm.GraphRoutingTest; +import org.opentripplanner.routing.api.request.RouteRequest; +import org.opentripplanner.routing.api.request.request.StreetRequest; +import org.opentripplanner.street.model.vertex.TransitStopVertex; + +class StreetNearbyStopFinderMultipleLinksTest extends GraphRoutingTest { + + private static final WgsCoordinate origin = new WgsCoordinate(0.0, 0.0); + private TransitStopVertex stopA; + private TransitStopVertex stopB; + private TransitStopVertex stopC; + + @BeforeEach + protected void setUp() throws Exception { + modelOf( + new Builder() { + @Override + public void build() { + var A = intersection("A", origin); + var B = intersection("B", origin.moveEastMeters(100)); + var C = intersection("C", origin.moveEastMeters(200)); + + biStreet(A, B, 100); + biStreet(B, C, 100); + + stopA = stop("StopA", A.toWgsCoordinate()); + stopB = stop("StopB", B.toWgsCoordinate()); + stopC = stop("StopC", C.toWgsCoordinate()); + + biLink(A, stopA); + + // B has many links + biLink(B, stopB); + biLink(B, stopB); + biLink(B, stopB); + biLink(B, stopB); + + biLink(C, stopC); + } + } + ); + } + + @Test + void testMaxStopCountRegression() { + // Max-stop-count should work correctly even though there are multiple links B <-> stopB + var durationLimit = Duration.ofMinutes(10); + var maxStopCount = 3; + var finder = new StreetNearbyStopFinder(durationLimit, maxStopCount, null); + + var sortedNearbyStops = sort( + finder.findNearbyStops(stopA, new RouteRequest(), new StreetRequest(), false) + ); + + assertThat(sortedNearbyStops).hasSize(3); + assertZeroDistanceStop(stopA, sortedNearbyStops.get(0)); + assertStopAtDistance(stopB, 100, sortedNearbyStops.get(1)); + assertStopAtDistance(stopC, 200, sortedNearbyStops.get(2)); + } +} diff --git a/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java b/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java index 261a40454f0..aae02451b33 100644 --- a/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java +++ b/application/src/test/java/org/opentripplanner/graph_builder/module/nearbystops/StreetNearbyStopFinderTest.java @@ -99,7 +99,6 @@ void testMultipleStops() { } @Test - @Disabled("Currently disabled because of a bug in stop counting") void testMaxStopCount() { var durationLimit = Duration.ofMinutes(10); var maxStopCount = 2; @@ -150,7 +149,6 @@ void testIgnoreStops() { } @Test - @Disabled("Currently disabled because of a bug in stop counting") void testIgnoreStopsWithMaxStops() { var durationLimit = Duration.ofMinutes(10); var maxStopCount = 1; @@ -165,14 +163,14 @@ void testIgnoreStopsWithMaxStops() { assertStopAtDistance(stopC, 200, sortedNearbyStops.get(0)); } - private List sort(Collection stops) { + static List sort(Collection stops) { return stops.stream().sorted(Comparator.comparing(x -> x.distance)).toList(); } /** * Verify that the nearby stop is zero distance and corresponds to the expected vertex */ - private void assertZeroDistanceStop(TransitStopVertex expected, NearbyStop nearbyStop) { + static void assertZeroDistanceStop(TransitStopVertex expected, NearbyStop nearbyStop) { assertEquals(expected.getStop(), nearbyStop.stop); assertEquals(0, nearbyStop.distance); assertEquals(0, nearbyStop.edges.size()); @@ -183,7 +181,7 @@ private void assertZeroDistanceStop(TransitStopVertex expected, NearbyStop nearb /** * Verify that the nearby stop is at a specific distance and corresponds to the expected vertex */ - private void assertStopAtDistance( + static void assertStopAtDistance( TransitStopVertex expected, double expectedDistance, NearbyStop nearbyStop