From 5e09389f3a09745929678186edcf979aba3d70a3 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 30 Sep 2024 22:51:51 +0200 Subject: [PATCH 1/4] Update stop consolidation logic --- .../DecorateConsolidatedStopNamesTest.java | 68 ++++++++++++++++--- .../DecorateConsolidatedStopNames.java | 41 +++++++++++ .../StopConsolidationModule.java | 2 +- .../StopConsolidationService.java | 3 +- .../DefaultStopConsolidationService.java | 9 ++- .../RouteRequestToFilterChainMapper.java | 4 +- 6 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java b/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java index 7f389bc41ec..eb37b274c8d 100644 --- a/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java +++ b/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java @@ -12,10 +12,14 @@ import org.opentripplanner.ext.stopconsolidation.internal.DefaultStopConsolidationRepository; import org.opentripplanner.ext.stopconsolidation.internal.DefaultStopConsolidationService; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup; +import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopLeg; import org.opentripplanner.model.fare.FareProduct; import org.opentripplanner.model.fare.FareProductUse; +import org.opentripplanner.model.plan.Leg; import org.opentripplanner.model.plan.Place; import org.opentripplanner.model.plan.PlanTestConstants; +import org.opentripplanner.model.plan.ScheduledTransitLeg; +import org.opentripplanner.model.plan.StreetLeg; import org.opentripplanner.model.plan.TestItineraryBuilder; import org.opentripplanner.transit.model.basic.Money; @@ -32,21 +36,18 @@ class DecorateConsolidatedStopNamesTest { private static final List fpu = List.of( new FareProductUse("c1a04702-1fb6-32d4-ba02-483bf68111ed", fp) ); + private static final List GROUPS = List.of( + new ConsolidatedStopGroup(STOP_C.getId(), List.of(STOP_D.getId())) + ); + private static final Place PLACE_C = Place.forStop(STOP_C); @Test void changeNames() { - var transitModel = TestStopConsolidationModel.buildTransitModel(); - - var groups = List.of(new ConsolidatedStopGroup(STOP_C.getId(), List.of(STOP_D.getId()))); - var repo = new DefaultStopConsolidationRepository(); - repo.addGroups(groups); - - var service = new DefaultStopConsolidationService(repo, transitModel); - var filter = new DecorateConsolidatedStopNames(service); + var filter = defaultFilter(); var itinerary = TestItineraryBuilder - .newItinerary(Place.forStop(STOP_C)) - .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, Place.forStop(STOP_C)) + .newItinerary(PLACE_C) + .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, PLACE_C) .bus(1, T11_05, T11_12, PlanTestConstants.E) .bus(1, T11_05, T11_12, PlanTestConstants.F) .build(); @@ -62,4 +63,51 @@ void changeNames() { // Check that the fares were carried over assertEquals(fpu, updatedLeg.fareProducts()); } + + @Test + void removeTransferAtConsolidatedStop() { + final var filter = defaultFilter(); + + var itinerary = TestItineraryBuilder + .newItinerary(PLACE_C) + .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, PLACE_C) + .walk(1, PLACE_C) + .bus(1, T11_05, T11_12, PlanTestConstants.F) + .build(); + + filter.decorate(itinerary); + + var legs = itinerary.getLegs().stream().map(Leg::getClass).toList(); + assertEquals(List.of(ConsolidatedStopLeg.class, ScheduledTransitLeg.class), legs); + } + + @Test + void keepRegularTransfer() { + final var filter = defaultFilter(); + + var itinerary = TestItineraryBuilder + .newItinerary(PLACE_C) + .bus(TestStopConsolidationModel.ROUTE, 1, T11_05, T11_12, PLACE_C) + .walk(1, PlanTestConstants.E) + .bus(1, T11_05, T11_12, PlanTestConstants.F) + .build(); + + filter.decorate(itinerary); + + var legs = itinerary.getLegs().stream().map(Leg::getClass).toList(); + assertEquals( + List.of(ConsolidatedStopLeg.class, StreetLeg.class, ScheduledTransitLeg.class), + legs + ); + } + + private static DecorateConsolidatedStopNames defaultFilter() { + var transitModel = TestStopConsolidationModel.buildTransitModel(); + + var repo = new DefaultStopConsolidationRepository(); + repo.addGroups(GROUPS); + + var service = new DefaultStopConsolidationService(repo, transitModel); + return new DecorateConsolidatedStopNames(service); + } } diff --git a/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java b/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java index 1806b3e9e32..dc338d17faa 100644 --- a/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java +++ b/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java @@ -1,8 +1,10 @@ package org.opentripplanner.ext.stopconsolidation; +import java.util.ArrayList; import java.util.Objects; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopLeg; import org.opentripplanner.model.plan.Itinerary; +import org.opentripplanner.model.plan.Leg; import org.opentripplanner.model.plan.ScheduledTransitLeg; import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryDecorator; @@ -13,6 +15,7 @@ */ public class DecorateConsolidatedStopNames implements ItineraryDecorator { + private static final int MAX_INTRA_STOP_WALK_DISTANCE_METERS = 15; private final StopConsolidationService service; public DecorateConsolidatedStopNames(StopConsolidationService service) { @@ -22,6 +25,7 @@ public DecorateConsolidatedStopNames(StopConsolidationService service) { @Override public void decorate(Itinerary itinerary) { replaceConsolidatedStops(itinerary); + removeWalkLegs(itinerary); } /** @@ -51,6 +55,43 @@ private void replaceConsolidatedStops(Itinerary i) { }); } + /** + * Removes walk legs from and to a consolidated stop if they are deemed "short". This means that + * they are from a different element of the consolidated stop. + */ + private void removeWalkLegs(Itinerary itinerary) { + var legs = new ArrayList<>(itinerary.getLegs()); + var first = legs.getFirst(); + if ( + service.isPartOfConsolidatedStop(first.getTo().stop) && + first.isWalkingLeg() && + first.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS + ) { + legs.removeFirst(); + } + var last = legs.getLast(); + if ( + service.isPartOfConsolidatedStop(last.getFrom().stop) && + last.isWalkingLeg() && + last.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS + ) { + legs.removeLast(); + } + + var transfersRemoved = legs.stream().filter(l -> !isTransferWithinConsolidatedStop(l)).toList(); + + itinerary.setLegs(transfersRemoved); + } + + private boolean isTransferWithinConsolidatedStop(Leg l) { + return ( + l.isWalkingLeg() && + (l.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS) && + service.isPartOfConsolidatedStop(l.getFrom().stop) && + service.isPartOfConsolidatedStop(l.getTo().stop) + ); + } + /** * Figures out if the from/to stops are part of a consolidated stop group and therefore * some stops need to be replaced. diff --git a/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java b/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java index 100941d88d4..ca04f442043 100644 --- a/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java +++ b/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModule.java @@ -26,7 +26,7 @@ */ public class StopConsolidationModule implements GraphBuilderModule { - private static final Logger LOG = LoggerFactory.getLogger(TripPattern.class); + private static final Logger LOG = LoggerFactory.getLogger(StopConsolidationModule.class); private final StopConsolidationRepository repository; private final TransitModel transitModel; diff --git a/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java b/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java index 68efe8744cc..0457212e66a 100644 --- a/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java +++ b/src/ext/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationService.java @@ -2,6 +2,7 @@ import java.util.List; import java.util.Optional; +import javax.annotation.Nullable; import org.opentripplanner.ext.stopconsolidation.model.StopReplacement; import org.opentripplanner.transit.model.framework.FeedScopedId; import org.opentripplanner.transit.model.organization.Agency; @@ -44,5 +45,5 @@ public interface StopConsolidationService { */ Optional primaryStop(FeedScopedId id); - boolean isPartOfConsolidatedStop(StopLocation sl); + boolean isPartOfConsolidatedStop(@Nullable StopLocation sl); } diff --git a/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java b/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java index 9f31e366be5..1bc558e83ac 100644 --- a/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java +++ b/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java @@ -5,6 +5,7 @@ import java.util.Optional; import java.util.stream.Stream; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import org.opentripplanner.ext.stopconsolidation.StopConsolidationRepository; import org.opentripplanner.ext.stopconsolidation.StopConsolidationService; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup; @@ -68,8 +69,12 @@ public boolean isSecondaryStop(StopLocation stop) { } @Override - public boolean isPartOfConsolidatedStop(StopLocation sl) { - return isSecondaryStop(sl) || isPrimaryStop(sl); + public boolean isPartOfConsolidatedStop(@Nullable StopLocation sl) { + if (sl == null) { + return false; + } else { + return isSecondaryStop(sl) || isPrimaryStop(sl); + } } @Override diff --git a/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java b/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java index c1fab68f999..2c36e375d28 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java +++ b/src/main/java/org/opentripplanner/routing/algorithm/mapping/RouteRequestToFilterChainMapper.java @@ -113,7 +113,9 @@ public static ItineraryListFilterChain createFilterChain( builder.withEmissions(new DecorateWithEmission(context.emissionsService())); } - if (context.stopConsolidationService() != null) { + if ( + context.stopConsolidationService() != null && context.stopConsolidationService().isActive() + ) { builder.withConsolidatedStopNamesDecorator( new DecorateConsolidatedStopNames(context.stopConsolidationService()) ); From 76bf428457d5cebcdd3898b6b5c5a4bac716610a Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 15 Oct 2024 12:09:41 +0200 Subject: [PATCH 2/4] Rename method --- .../ext/stopconsolidation/DecorateConsolidatedStopNames.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java index dc338d17faa..0ba844bf3fa 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java @@ -25,7 +25,7 @@ public DecorateConsolidatedStopNames(StopConsolidationService service) { @Override public void decorate(Itinerary itinerary) { replaceConsolidatedStops(itinerary); - removeWalkLegs(itinerary); + removeShortWalkLegs(itinerary); } /** @@ -59,7 +59,7 @@ private void replaceConsolidatedStops(Itinerary i) { * Removes walk legs from and to a consolidated stop if they are deemed "short". This means that * they are from a different element of the consolidated stop. */ - private void removeWalkLegs(Itinerary itinerary) { + private void removeShortWalkLegs(Itinerary itinerary) { var legs = new ArrayList<>(itinerary.getLegs()); var first = legs.getFirst(); if ( From 7a9996fcf885b41f6ad96cf14b89abc2d8fb1982 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 15 Oct 2024 12:44:42 +0200 Subject: [PATCH 3/4] Extract logic --- .../DecorateConsolidatedStopNames.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java index 0ba844bf3fa..15bc93a5678 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java @@ -64,16 +64,14 @@ private void removeShortWalkLegs(Itinerary itinerary) { var first = legs.getFirst(); if ( service.isPartOfConsolidatedStop(first.getTo().stop) && - first.isWalkingLeg() && - first.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS + isShortWalkLeg(first) ) { legs.removeFirst(); } var last = legs.getLast(); if ( service.isPartOfConsolidatedStop(last.getFrom().stop) && - last.isWalkingLeg() && - last.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS + isShortWalkLeg(last) ) { legs.removeLast(); } @@ -83,6 +81,11 @@ private void removeShortWalkLegs(Itinerary itinerary) { itinerary.setLegs(transfersRemoved); } + private static boolean isShortWalkLeg(Leg leg) { + return leg.isWalkingLeg() && + leg.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS; + } + private boolean isTransferWithinConsolidatedStop(Leg l) { return ( l.isWalkingLeg() && From 09ae95d01fd5a796602e506d08fbd7de58b196cf Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 15 Oct 2024 14:28:52 +0200 Subject: [PATCH 4/4] Re-use method --- .../DecorateConsolidatedStopNames.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java index 15bc93a5678..6def5c85fa1 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java @@ -81,20 +81,17 @@ private void removeShortWalkLegs(Itinerary itinerary) { itinerary.setLegs(transfersRemoved); } + private boolean isTransferWithinConsolidatedStop(Leg l) { + return isShortWalkLeg(l) && + service.isPartOfConsolidatedStop(l.getFrom().stop) && + service.isPartOfConsolidatedStop(l.getTo().stop); + } + private static boolean isShortWalkLeg(Leg leg) { return leg.isWalkingLeg() && leg.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS; } - private boolean isTransferWithinConsolidatedStop(Leg l) { - return ( - l.isWalkingLeg() && - (l.getDistanceMeters() < MAX_INTRA_STOP_WALK_DISTANCE_METERS) && - service.isPartOfConsolidatedStop(l.getFrom().stop) && - service.isPartOfConsolidatedStop(l.getTo().stop) - ); - } - /** * Figures out if the from/to stops are part of a consolidated stop group and therefore * some stops need to be replaced.