Merge pull request #4388 from entur/otp2_netex_verify_stoptimes

Rename and clean up RepairStopTimesForEachTripOperation

src/ext-test/java/org/opentripplanner/ext/flex/ScheduledDeviatedTripTest.java

@@ -20,6 +20,7 @@
 import org.opentripplanner.ext.fares.FaresFilter;
 import org.opentripplanner.ext.flex.trip.FlexTrip;
 import org.opentripplanner.ext.flex.trip.ScheduledDeviatedTrip;
+import org.opentripplanner.graph_builder.module.ValidateAndInterpolateStopTimesForEachTrip;
 import org.opentripplanner.model.GenericLocation;
 import org.opentripplanner.model.StopTime;
 import org.opentripplanner.model.plan.Itinerary;
@@ -196,7 +197,7 @@ void flexTripInTransitMode() {
    * Normally these trip times are interpolated/repaired during the graph build but for flex this is
    * exactly what we don't want. Here we check that the interpolation process is skipped.
    *
-   * @see org.opentripplanner.gtfs.RepairStopTimesForEachTripOperation#interpolateStopTimes(List)
+   * @see ValidateAndInterpolateStopTimesForEachTrip#interpolateStopTimes(List)
    */
   @Test
   void shouldNotInterpolateFlexTimes() {

src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java

@@ -32,7 +32,6 @@
 import org.opentripplanner.graph_builder.module.geometry.GeometryProcessor;
 import org.opentripplanner.graph_builder.module.interlining.InterlineProcessor;
 import org.opentripplanner.gtfs.GenerateTripPatternsOperation;
-import org.opentripplanner.gtfs.RepairStopTimesForEachTripOperation;
 import org.opentripplanner.gtfs.mapping.GTFSToOtpTransitServiceMapper;
 import org.opentripplanner.model.OtpTransitService;
 import org.opentripplanner.model.TripStopTimes;
@@ -138,7 +137,7 @@ public void buildGraph() {
           builder.getFlexTripsById().addAll(FlexTripsMapper.createFlexTrips(builder, issueStore));
         }
 
-        repairStopTimesForEachTrip(builder.getStopTimesSortedByTrip(), issueStore);
+        validateAndInterpolateStopTimesForEachTrip(builder.getStopTimesSortedByTrip(), issueStore);
 
         GeometryProcessor geometryProcessor = new GeometryProcessor(
           builder,
@@ -201,11 +200,11 @@ public void checkInputs() {
   /**
    * This method has side effects, the {@code stopTimesByTrip} is updated.
    */
-  private void repairStopTimesForEachTrip(
+  private void validateAndInterpolateStopTimesForEachTrip(
     TripStopTimes stopTimesByTrip,
     DataImportIssueStore issueStore
   ) {
-    new RepairStopTimesForEachTripOperation(stopTimesByTrip, issueStore).run();
+    new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, true, issueStore).run();
   }
 
   /**

src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java → src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java

@@ -1,4 +1,4 @@
-package org.opentripplanner.gtfs;
+package org.opentripplanner.graph_builder.module;
 
 import gnu.trove.list.TIntList;
 import gnu.trove.list.array.TIntArrayList;
@@ -21,41 +21,41 @@
 import org.opentripplanner.transit.model.site.RegularStop;
 import org.opentripplanner.transit.model.timetable.Trip;
 import org.opentripplanner.util.OTPFeature;
+import org.opentripplanner.util.logging.ProgressTracker;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * This class is responsible for cleaning stop times, removing duplicates, correcting bad data and
- * so on. This only applies to GTFS imports.
+ * This class is responsible for making sure, that all trips have arrival and departure times for
+ * all stops. It also removes all stop times for trips, which have invalid stop times.
  */
-public class RepairStopTimesForEachTripOperation {
+public class ValidateAndInterpolateStopTimesForEachTrip {
 
   private static final Logger LOG = LoggerFactory.getLogger(
-    RepairStopTimesForEachTripOperation.class
+    ValidateAndInterpolateStopTimesForEachTrip.class
   );
 
   private final TripStopTimes stopTimesByTrip;
-
+  private final boolean interpolate;
   private final DataImportIssueStore issueStore;
 
-  public RepairStopTimesForEachTripOperation(
+  public ValidateAndInterpolateStopTimesForEachTrip(
     TripStopTimes stopTimesByTrip,
+    boolean interpolate,
     DataImportIssueStore issueStore
   ) {
     this.stopTimesByTrip = stopTimesByTrip;
+    this.interpolate = interpolate;
     this.issueStore = issueStore;
   }
 
   public void run() {
     final int tripSize = stopTimesByTrip.size();
-    int tripCount = 0;
+    var progress = ProgressTracker.track("Validate StopTimes", 100_000, tripSize);
+    LOG.info(progress.startMessage());
 
     for (Trip trip : stopTimesByTrip.keys()) {
-      if (++tripCount % 100000 == 0) {
-        LOG.debug("Repair StopTimes for trips {}/{}", tripCount, tripSize);
-      }
-
-      /* Fetch the stop times for this trip. Copy the list since it's immutable. */
+      // Fetch the stop times for this trip. Copy the list since it's immutable.
       List<StopTime> stopTimes = new ArrayList<>(stopTimesByTrip.get(trip));
 
       // if we don't have flex routing enabled then remove all the flex locations and location
@@ -64,27 +64,33 @@ public void run() {
         stopTimes.removeIf(st -> !(st.getStop() instanceof RegularStop));
       }
 
-      /* Stop times frequently contain duplicate, missing, or incorrect entries. Repair them. */
+      // Stop times frequently contain duplicate, missing, or incorrect entries. Repair them.
       TIntList removedStopSequences = removeRepeatedStops(stopTimes);
       if (!removedStopSequences.isEmpty()) {
         issueStore.add(new RepeatedStops(trip, removedStopSequences));
       }
       if (!filterStopTimes(stopTimes)) {
         stopTimesByTrip.replace(trip, List.of());
-      } else {
+      } else if (interpolate) {
         interpolateStopTimes(stopTimes);
         stopTimesByTrip.replace(trip, stopTimes);
+      } else {
+        stopTimes.removeIf(st -> !st.isArrivalTimeSet() || !st.isDepartureTimeSet());
+        stopTimesByTrip.replace(trip, stopTimes);
       }
+
+      //noinspection Convert2MethodRef
+      progress.step(m -> LOG.info(m));
     }
+
+    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>
-   * TODO: OBA GFTS makes the stoptime lists unmodifiable, so this will not work.
-   * We need to copy any modified list.
    *
    * @return whether any repeated stops were filtered out.
    */
@@ -96,18 +102,16 @@ private TIntList removeRepeatedStops(List<StopTime> stopTimes) {
       StopTime st = it.next();
       if (prev != null) {
         if (prev.getStop().equals(st.getStop())) {
-          // OBA gives us unmodifiable lists, but we have copied them.
-
-          // 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)
+          // 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
+          // 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());
           }
@@ -122,11 +126,11 @@ private TIntList removeRepeatedStops(List<StopTime> stopTimes) {
   }
 
   /**
-   * Scan through the given list, looking for clearly incorrect series of stoptimes. This includes
+   * 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
    * are reported to reveal the problems to the user.
    *
-   * @param stopTimes the stoptimes to be filtered (from a single trip)
+   * @param stopTimes the stop times to be filtered (from a single trip)
    * @return whether the stop time is usable
    */
   private boolean filterStopTimes(List<StopTime> stopTimes) {
@@ -136,31 +140,32 @@ private boolean filterStopTimes(List<StopTime> stopTimes) {
 
     StopTime st0 = stopTimes.get(0);
 
-    /* If the feed does not specify any timepoints, we want to mark all times that are present as timepoints. */
+    // If the feed does not specify any time points, we want to mark all times that are present as
+    // time points.
     boolean hasTimepoints = stopTimes.stream().anyMatch(stopTime -> stopTime.getTimepoint() == 1);
 
-    // TODO verify that the first (and last?) stop should always be considered a timepoint.
     if (!hasTimepoints) {
       st0.setTimepoint(1);
     }
 
     for (int i = 1; i < stopTimes.size(); i++) {
       StopTime st1 = stopTimes.get(i);
 
-      /* If the feed did not specify any timepoints, mark all times that are present as timepoints. */
+      // If the feed did not specify any time points, mark all times that are present as time points
       if (!hasTimepoints && (st1.isDepartureTimeSet() || st1.isArrivalTimeSet())) {
         st1.setTimepoint(1);
       }
 
-      /* Set arrival and departure times if either is missing. */
+      // Set arrival and departure times if one of them is missing.
       if (!st1.isArrivalTimeSet() && st1.isDepartureTimeSet()) {
         st1.setArrivalTime(st1.getDepartureTime());
       } else if (!st1.isDepartureTimeSet() && st1.isArrivalTimeSet()) {
         st1.setDepartureTime(st1.getArrivalTime());
       }
 
-      /* Do not process (skip over) non-timepoint stoptimes, leaving them in place for interpolation. */
-      // All non-timepoint stoptimes in a series will have identical arrival and departure values of MISSING_VALUE.
+      // Do not process non-time-point stop times, leaving them in place for interpolation.
+      // All non-timepoint stoptimes in a series will have identical arrival and departure values of
+      // MISSING_VALUE.
       if (!(st1.isArrivalTimeSet() && st1.isDepartureTimeSet())) {
         continue;
       }
@@ -172,7 +177,7 @@ private boolean filterStopTimes(List<StopTime> stopTimes) {
 
       int runningTime = st1.getArrivalTime() - st0.getDepartureTime();
       if (runningTime < 0) {
-        issueStore.add(new NegativeHopTime(new StopTime(st0), new StopTime(st1)));
+        issueStore.add(new NegativeHopTime(st0, st1));
         return false;
       }
 
@@ -187,7 +192,7 @@ private boolean filterStopTimes(List<StopTime> stopTimes) {
       }
       // sanity-check the hop
       if (runningTime == 0) {
-        // series of identical stop times at different stops
+        // identical stop times at different stops
         issueStore.add(new HopZeroTime((float) hopDistance, st1.getTrip(), st1.getStopSequence()));
       } else if (hopSpeed > 45) {
         // 45 m/sec ~= 100 miles/hr
@@ -226,19 +231,18 @@ private boolean filterStopTimes(List<StopTime> stopTimes) {
    */
   private void interpolateStopTimes(List<StopTime> stopTimes) {
     int lastStop = stopTimes.size() - 1;
-    int numInterpStops = -1;
-    int departureTime = -1, prevDepartureTime = -1;
-    int interpStep = 0;
+    int numInterpStops;
+    int departureTime = -1;
+    int prevDepartureTime;
+    int interpStep;
 
-    int i;
-    for (i = 0; i < lastStop; i++) {
+    for (int i = 0; i < lastStop; i++) {
       StopTime st0 = stopTimes.get(i);
 
       prevDepartureTime = departureTime;
       departureTime = st0.getDepartureTime();
 
-      /* Interpolate, if necessary, the times of non-timepoint stops */
-      /* genuine interpolation needed */
+      // Interpolate, if necessary, the times of non-timepoint stops
       if (
         !(st0.isDepartureTimeSet() && st0.isArrivalTimeSet()) && !FlexTrip.isFlexStop(st0.getStop())
       ) {

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

@@ -6,7 +6,9 @@
 import org.opentripplanner.graph_builder.model.GraphBuilderModule;
 import org.opentripplanner.graph_builder.module.AddTransitModelEntitiesToGraph;
 import org.opentripplanner.graph_builder.module.GtfsFeedId;
+import org.opentripplanner.graph_builder.module.ValidateAndInterpolateStopTimesForEachTrip;
 import org.opentripplanner.model.OtpTransitService;
+import org.opentripplanner.model.TripStopTimes;
 import org.opentripplanner.model.calendar.CalendarServiceData;
 import org.opentripplanner.model.calendar.ServiceDateInterval;
 import org.opentripplanner.model.impl.OtpTransitServiceBuilder;
@@ -81,6 +83,8 @@ public void buildGraph() {
             .addAll(FlexTripsMapper.createFlexTrips(transitBuilder, issueStore));
         }
 
+        validateStopTimesForEachTrip(transitBuilder.getStopTimesSortedByTrip());
+
         OtpTransitService otpService = transitBuilder.build();
 
         // if this or previously processed netex bundle has transit that has not been filtered out
@@ -111,6 +115,10 @@ public void buildGraph() {
     }
   }
 
+  private void validateStopTimesForEachTrip(TripStopTimes stopTimesByTrip) {
+    new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, false, issueStore).run();
+  }
+
   @Override
   public void checkInputs() {
     netexBundles.forEach(NetexBundle::checkInputs);

src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java

@@ -6,6 +6,7 @@
 import org.opentripplanner.graph_builder.DataImportIssueStore;
 import org.opentripplanner.graph_builder.module.GtfsFeedId;
 import org.opentripplanner.graph_builder.module.GtfsModule;
+import org.opentripplanner.graph_builder.module.ValidateAndInterpolateStopTimesForEachTrip;
 import org.opentripplanner.graph_builder.module.geometry.GeometryProcessor;
 import org.opentripplanner.gtfs.mapping.GTFSToOtpTransitServiceMapper;
 import org.opentripplanner.model.OtpTransitService;
@@ -132,7 +133,11 @@ private static GtfsImport gtfsImport(String defaultFeedId, String path) throws I
   }
 
   private void repairStopTimesForEachTrip() {
-    new RepairStopTimesForEachTripOperation(transitBuilder.getStopTimesSortedByTrip(), issueStore)
+    new ValidateAndInterpolateStopTimesForEachTrip(
+      transitBuilder.getStopTimesSortedByTrip(),
+      true,
+      issueStore
+    )
       .run();
   }