Merge pull request #6846 from ibi-group/remove-repeated-stops

Remove GTFS "remove repeated stops" feature

Author: leonardehrenfried

application/src/ext-test/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTripIntegrationTest.java

@@ -113,13 +113,13 @@ void flexTripInTransitMode() {
     assertEquals("yz85", leg.to().stop.getId().getId());
 
     var intermediateStops = leg.listIntermediateStops();
-    assertEquals(1, intermediateStops.size());
+    assertEquals(2, intermediateStops.size());
     assertEquals("zone_1", intermediateStops.get(0).place.stop.getId().getId());
 
     EncodedPolyline legGeometry = EncodedPolyline.encode(leg.legGeometry());
     assertThatPolylinesAreEqual(
       legGeometry.points(),
-      "kfsmEjojcOa@eBRKfBfHR|ALjBBhVArMG|OCrEGx@OhAKj@a@tAe@hA]l@MPgAnAgw@nr@cDxCm@t@c@t@c@x@_@~@]pAyAdIoAhG}@lE{AzHWhAtt@t~Aj@tAb@~AXdBHn@FlBC`CKnA_@nC{CjOa@dCOlAEz@E|BRtUCbCQ~CWjD??qBvXBl@kBvWOzAc@dDOx@sHv]aIG?q@@c@ZaB\\mA"
+      "kfsmEjojcOa@eBRKfBfHR|ALjBBhVArMG|OCrEGx@OhAKj@a@tAe@hA]l@MPgAnAgw@nr@cDxCm@t@c@t@c@x@_@~@]pAyAdIoAhG}@lE{AzHWhAtt@t~Aj@tAb@~AXdBHn@FlBC`CKnA_@nC{CjOa@dCOlAEz@E|BRtUCbCQ~CWjD??????qBvXBl@kBvWOzAc@dDOx@sHv]aIG?q@@c@ZaB\\mA"
     );
   }
 
@@ -136,7 +136,7 @@ void shouldNotInterpolateFlexTimes() {
     var feedId = timetableRepository.getFeedIds().iterator().next();
     var pattern = timetableRepository.getTripPatternForId(new FeedScopedId(feedId, "090z:0:01"));
 
-    assertEquals(3, pattern.numberOfStops());
+    assertEquals(4, pattern.numberOfStops());
 
     var tripTimes = pattern.getScheduledTimetable().getTripTimes().getFirst();
     var arrivalTime = tripTimes.getArrivalTime(1);

application/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java

@@ -1,9 +1,6 @@
 package org.opentripplanner.graph_builder.module;
 
-import gnu.trove.list.TIntList;
-import gnu.trove.list.array.TIntArrayList;
 import java.util.ArrayList;
-import java.util.Iterator;
 import java.util.List;
 import org.opentripplanner.ext.flex.trip.FlexTrip;
 import org.opentripplanner.framework.application.OTPFeature;
@@ -16,7 +13,6 @@
 import org.opentripplanner.graph_builder.issues.HopZeroTime;
 import org.opentripplanner.graph_builder.issues.NegativeDwellTime;
 import org.opentripplanner.graph_builder.issues.NegativeHopTime;
-import org.opentripplanner.graph_builder.issues.RepeatedStops;
 import org.opentripplanner.model.StopTime;
 import org.opentripplanner.model.TripStopTimes;
 import org.opentripplanner.transit.model.basic.TransitMode;
@@ -41,18 +37,15 @@ public class ValidateAndInterpolateStopTimesForEachTrip {
 
   private final TripStopTimes stopTimesByTrip;
   private final boolean interpolate;
-  private final boolean removeRepeatedStops;
   private final DataImportIssueStore issueStore;
 
   public ValidateAndInterpolateStopTimesForEachTrip(
     TripStopTimes stopTimesByTrip,
     boolean interpolate,
-    boolean removeRepeatedStops,
     DataImportIssueStore issueStore
   ) {
     this.stopTimesByTrip = stopTimesByTrip;
     this.interpolate = interpolate;
-    this.removeRepeatedStops = removeRepeatedStops;
     this.issueStore = issueStore;
   }
 
@@ -70,12 +63,7 @@ public void run() {
       if (OTPFeature.FlexRouting.isOff()) {
         stopTimes.removeIf(st -> !(st.getStop() instanceof RegularStop));
       }
-
-      // Stop times frequently contain duplicate, missing, or incorrect entries. Repair them.
-      TIntList removedStopSequences = removeRepeatedStops(stopTimes);
-      if (!removedStopSequences.isEmpty()) {
-        issueStore.add(new RepeatedStops(trip, removedStopSequences));
-      }
+      // Stop times frequently contain missing, or incorrect entries. Repair them.
       if (!filterStopTimes(stopTimes)) {
         stopTimesByTrip.replace(trip, List.of());
       } else if (interpolate) {
@@ -93,47 +81,6 @@ public void run() {
     LOG.info(progress.completeMessage());
   }
 
-  /**
-   * Filter out any series of stop times that refer to the same stop. This is very inefficient in an
-   * array-backed list, but we are assuming that this is a rare occurrence. The alternative is to
-   * copy every list of stop times during filtering.
-   * <p>
-   *
-   * @return whether any repeated stops were filtered out.
-   */
-  private TIntList removeRepeatedStops(List<StopTime> stopTimes) {
-    StopTime prev = null;
-    Iterator<StopTime> it = stopTimes.iterator();
-    TIntList stopSequencesRemoved = new TIntArrayList();
-    while (it.hasNext()) {
-      StopTime st = it.next();
-      if (prev != null) {
-        if (prev.getStop().equals(st.getStop())) {
-          if (removeRepeatedStops) {
-            // Merge the two stop times, making sure we're not throwing out a stop time with times in
-            // favor of an interpolated stop time. Keep the arrival time of the previous stop, unless
-            // it didn't have an arrival time, in which case replace it with the arrival time of this
-            // stop time. This is particularly important at the last stop in a route (see issue #2220)
-            if (prev.getArrivalTime() == StopTime.MISSING_VALUE) {
-              prev.setArrivalTime(st.getArrivalTime());
-            }
-
-            // prefer to replace with the departure time of this stop time, unless this stop time has
-            // no departure time
-            if (st.getDepartureTime() != StopTime.MISSING_VALUE) {
-              prev.setDepartureTime(st.getDepartureTime());
-            }
-
-            it.remove();
-          }
-          stopSequencesRemoved.add(st.getStopSequence());
-        }
-      }
-      prev = st;
-    }
-    return stopSequencesRemoved;
-  }
-
   /**
    * Scan through the given list, looking for clearly incorrect series of stop times. This includes
    * duplicate times (0-time hops), as well as negative, fast or slow hops. {@link DataImportIssue}s

application/src/main/java/org/opentripplanner/gtfs/config/GtfsDefaultParameters.java

@@ -11,33 +11,28 @@
  */
 public sealed class GtfsDefaultParameters permits GtfsFeedParameters {
 
-  private static final boolean DEFAULT_REMOVE_REPEATED_STOPS = true;
   private static final boolean DEFAULT_DISCARD_MIN_TRANSFER_TIMES = false;
   private static final boolean DEFAULT_BLOCK_BASED_INTERLINING = true;
   private static final int DEFAULT_MAX_INTERLINE_DISTANCE = 200;
 
   public static final GtfsDefaultParameters DEFAULT = new GtfsDefaultParameters(
-    DEFAULT_REMOVE_REPEATED_STOPS,
     StopTransferPriority.defaultValue(),
     DEFAULT_DISCARD_MIN_TRANSFER_TIMES,
     DEFAULT_BLOCK_BASED_INTERLINING,
     DEFAULT_MAX_INTERLINE_DISTANCE
   );
 
-  private final boolean removeRepeatedStops;
   private final StopTransferPriority stationTransferPreference;
   private final boolean discardMinTransferTimes;
   private final boolean blockBasedInterlining;
   private final int maxInterlineDistance;
 
   protected GtfsDefaultParameters(
-    boolean removeRepeatedStops,
     StopTransferPriority stationTransferPreference,
     boolean discardMinTransferTimes,
     boolean blockBasedInterlining,
     int maxInterlineDistance
   ) {
-    this.removeRepeatedStops = removeRepeatedStops;
     this.stationTransferPreference = Objects.requireNonNull(stationTransferPreference);
     this.discardMinTransferTimes = discardMinTransferTimes;
     this.blockBasedInterlining = blockBasedInterlining;
@@ -46,7 +41,6 @@ protected GtfsDefaultParameters(
 
   GtfsDefaultParameters(GtfsDefaultParametersBuilder builder) {
     this(
-      builder.removeRepeatedStops(),
       builder.stationTransferPreference(),
       builder.discardMinTransferTimes(),
       builder.blockBasedInterlining(),
@@ -65,10 +59,6 @@ public GtfsFeedParametersBuilder withFeedInfo() {
     return new GtfsFeedParametersBuilder(this);
   }
 
-  public boolean removeRepeatedStops() {
-    return removeRepeatedStops;
-  }
-
   public StopTransferPriority stationTransferPreference() {
     return stationTransferPreference;
   }
@@ -106,7 +96,6 @@ public String toString() {
 
   protected ToStringBuilder toStringAppend(ToStringBuilder builder) {
     return builder
-      .addBool("removeRepeatedStops", removeRepeatedStops, DEFAULT_REMOVE_REPEATED_STOPS)
       .addEnum(
         "stationTransferPreference",
         stationTransferPreference,

application/src/main/java/org/opentripplanner/gtfs/config/GtfsDefaultParametersBuilder.java

@@ -4,14 +4,12 @@
 
 public class GtfsDefaultParametersBuilder {
 
-  private boolean removeRepeatedStops;
   private StopTransferPriority stationTransferPreference;
   private boolean discardMinTransferTimes;
   private boolean blockBasedInterlining;
   private int maxInterlineDistance;
 
   public GtfsDefaultParametersBuilder(GtfsDefaultParameters original) {
-    this.removeRepeatedStops = original.removeRepeatedStops();
     this.stationTransferPreference = original.stationTransferPreference();
     this.discardMinTransferTimes = original.discardMinTransferTimes();
     this.blockBasedInterlining = original.blockBasedInterlining();
@@ -29,15 +27,6 @@ StopTransferPriority stationTransferPreference() {
     return stationTransferPreference;
   }
 
-  public GtfsDefaultParametersBuilder withRemoveRepeatedStops(boolean value) {
-    this.removeRepeatedStops = value;
-    return this;
-  }
-
-  boolean removeRepeatedStops() {
-    return removeRepeatedStops;
-  }
-
   public GtfsDefaultParametersBuilder withDiscardMinTransferTimes(boolean value) {
     this.discardMinTransferTimes = value;
     return this;

application/src/main/java/org/opentripplanner/gtfs/config/GtfsFeedParameters.java

@@ -22,14 +22,12 @@ public final class GtfsFeedParameters extends GtfsDefaultParameters implements D
   public GtfsFeedParameters(
     @Nullable String feedId,
     URI source,
-    boolean removeRepeatedStops,
     StopTransferPriority stationTransferPreference,
     boolean discardMinTransferTimes,
     boolean blockBasedInterlining,
     int maxInterlineDistance
   ) {
     super(
-      removeRepeatedStops,
       stationTransferPreference,
       discardMinTransferTimes,
       blockBasedInterlining,

application/src/main/java/org/opentripplanner/gtfs/config/GtfsFeedParametersBuilder.java

@@ -38,7 +38,6 @@ public GtfsFeedParameters build() {
     return new GtfsFeedParameters(
       feedId,
       source,
-      removeRepeatedStops(),
       stationTransferPreference(),
       discardMinTransferTimes(),
       blockBasedInterlining(),

application/src/main/java/org/opentripplanner/gtfs/graphbuilder/GtfsModule.java

@@ -160,11 +160,7 @@ public void buildGraph() {
           builder.getFlexTripsById().addAll(FlexTripsMapper.createFlexTrips(builder, issueStore));
         }
 
-        validateAndInterpolateStopTimesForEachTrip(
-          builder.getStopTimesSortedByTrip(),
-          issueStore,
-          gtfsBundle.parameters().removeRepeatedStops()
-        );
+        validateAndInterpolateStopTimesForEachTrip(builder.getStopTimesSortedByTrip(), issueStore);
 
         // We need to run this after the cleaning of the data, as stop indices might have changed
         mapper.mapAndAddTransfersToBuilder(gtfsDao);
@@ -253,15 +249,9 @@ public void checkInputs() {
    */
   private void validateAndInterpolateStopTimesForEachTrip(
     TripStopTimes stopTimesByTrip,
-    DataImportIssueStore issueStore,
-    boolean removeRepeatedStops
+    DataImportIssueStore issueStore
   ) {
-    new ValidateAndInterpolateStopTimesForEachTrip(
-      stopTimesByTrip,
-      true,
-      removeRepeatedStops,
-      issueStore
-    ).run();
+    new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, true, issueStore).run();
   }
 
   /**

application/src/main/java/org/opentripplanner/gtfs/mapping/AgencyAndIdMapper.java

@@ -124,7 +124,7 @@ public void buildGraph() {
   }
 
   private void validateStopTimesForEachTrip(TripStopTimes stopTimesByTrip) {
-    new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, false, false, issueStore).run();
+    new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, false, issueStore).run();
   }
 
   @Override

application/src/main/java/org/opentripplanner/netex/NetexModule.java

@@ -54,17 +54,6 @@ private static GtfsDefaultParameters mapGenericParameters(
     var docDefaults = GtfsFeedParameters.DEFAULT;
     return defaults
       .copyOf()
-      .withRemoveRepeatedStops(
-        node
-          .of("removeRepeatedStops")
-          .since(V2_3)
-          .summary(
-            "Should consecutive identical stops be merged into one stop time entry." +
-            documentationAddition
-          )
-          .docDefaultValue(docDefaults.removeRepeatedStops())
-          .asBoolean(defaults.removeRepeatedStops())
-      )
       .withStationTransferPreference(
         node
           .of("stationTransferPreference")