From 74343ec442fec9024696f3a379a5c2c329962eeb Mon Sep 17 00:00:00 2001 From: Hannes Junnila Date: Wed, 17 Aug 2022 17:30:53 +0300 Subject: [PATCH 1/4] Use ProgressTracker in RepairStopTimesForEachTripOperation --- .../gtfs/RepairStopTimesForEachTripOperation.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java b/src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java index a166039b486..88dea64d0ee 100644 --- a/src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java +++ b/src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java @@ -21,6 +21,7 @@ import org.opentripplanner.transit.model.site.Stop; 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; @@ -48,13 +49,10 @@ public RepairStopTimesForEachTripOperation( public void run() { final int tripSize = stopTimesByTrip.size(); - int tripCount = 0; + var progress = ProgressTracker.track("Repair 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. */ List stopTimes = new ArrayList<>(stopTimesByTrip.get(trip)); @@ -75,7 +73,12 @@ public void run() { interpolateStopTimes(stopTimes); stopTimesByTrip.replace(trip, stopTimes); } + + //noinspection Convert2MethodRef + progress.step(m -> LOG.info(m)); } + + LOG.info(progress.completeMessage()); } /** From a7cd68599b99ab2bfeb17029c26766018e4a0816 Mon Sep 17 00:00:00 2001 From: Hannes Junnila Date: Thu, 18 Aug 2022 14:02:45 +0300 Subject: [PATCH 2/4] Rename and move RepairStopTimesForEachTripOperation --- .../ext/flex/ScheduledDeviatedTripTest.java | 3 ++- .../opentripplanner/graph_builder/module/GtfsModule.java | 3 +-- .../ValidateAndInterpolateStopTimesForEachTrip.java} | 8 ++++---- .../java/org/opentripplanner/gtfs/GtfsContextBuilder.java | 6 +++++- 4 files changed, 12 insertions(+), 8 deletions(-) rename src/main/java/org/opentripplanner/{gtfs/RepairStopTimesForEachTripOperation.java => graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java} (98%) diff --git a/src/ext-test/java/org/opentripplanner/ext/flex/ScheduledDeviatedTripTest.java b/src/ext-test/java/org/opentripplanner/ext/flex/ScheduledDeviatedTripTest.java index c5d66fe11d2..f279d07a656 100644 --- a/src/ext-test/java/org/opentripplanner/ext/flex/ScheduledDeviatedTripTest.java +++ b/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() { diff --git a/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java b/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java index c170f595975..cea2b605808 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java +++ b/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; @@ -205,7 +204,7 @@ private void repairStopTimesForEachTrip( TripStopTimes stopTimesByTrip, DataImportIssueStore issueStore ) { - new RepairStopTimesForEachTripOperation(stopTimesByTrip, issueStore).run(); + new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, issueStore).run(); } /** diff --git a/src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java b/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java similarity index 98% rename from src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java rename to src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java index 88dea64d0ee..6dc1b988775 100644 --- a/src/main/java/org/opentripplanner/gtfs/RepairStopTimesForEachTripOperation.java +++ b/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; @@ -29,17 +29,17 @@ * This class is responsible for cleaning stop times, removing duplicates, correcting bad data and * so on. This only applies to GTFS imports. */ -public class RepairStopTimesForEachTripOperation { +public class ValidateAndInterpolateStopTimesForEachTrip { private static final Logger LOG = LoggerFactory.getLogger( - RepairStopTimesForEachTripOperation.class + ValidateAndInterpolateStopTimesForEachTrip.class ); private final TripStopTimes stopTimesByTrip; private final DataImportIssueStore issueStore; - public RepairStopTimesForEachTripOperation( + public ValidateAndInterpolateStopTimesForEachTrip( TripStopTimes stopTimesByTrip, DataImportIssueStore issueStore ) { diff --git a/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java b/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java index daf1c89ddc9..d3a9b428065 100644 --- a/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java +++ b/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,10 @@ private static GtfsImport gtfsImport(String defaultFeedId, String path) throws I } private void repairStopTimesForEachTrip() { - new RepairStopTimesForEachTripOperation(transitBuilder.getStopTimesSortedByTrip(), issueStore) + new ValidateAndInterpolateStopTimesForEachTrip( + transitBuilder.getStopTimesSortedByTrip(), + issueStore + ) .run(); } From f43c7b55bee756b60cb250e787e8f0675ef7185c Mon Sep 17 00:00:00 2001 From: Hannes Junnila Date: Thu, 18 Aug 2022 14:04:40 +0300 Subject: [PATCH 3/4] Clean up ValidateAndInterpolateStopTimesForEachTrip --- .../graph_builder/module/GtfsModule.java | 4 +- ...ateAndInterpolateStopTimesForEachTrip.java | 60 +++++++++---------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java b/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java index cea2b605808..4221813d26c 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java @@ -137,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, @@ -200,7 +200,7 @@ public void checkInputs() { /** * This method has side effects, the {@code stopTimesByTrip} is updated. */ - private void repairStopTimesForEachTrip( + private void validateAndInterpolateStopTimesForEachTrip( TripStopTimes stopTimesByTrip, DataImportIssueStore issueStore ) { diff --git a/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java b/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java index 6dc1b988775..501c3deecd7 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java @@ -26,8 +26,8 @@ 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 ValidateAndInterpolateStopTimesForEachTrip { @@ -49,11 +49,11 @@ public ValidateAndInterpolateStopTimesForEachTrip( public void run() { final int tripSize = stopTimesByTrip.size(); - var progress = ProgressTracker.track("Repair StopTimes", 100_000, tripSize); + var progress = ProgressTracker.track("Validate StopTimes", 100_000, tripSize); LOG.info(progress.startMessage()); for (Trip trip : stopTimesByTrip.keys()) { - /* 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 stopTimes = new ArrayList<>(stopTimesByTrip.get(trip)); // if we don't have flex routing enabled then remove all the flex locations and location @@ -62,7 +62,7 @@ public void run() { stopTimes.removeIf(st -> !(st.getStop() instanceof Stop)); } - /* 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)); @@ -86,8 +86,6 @@ public void run() { * 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. *

- * 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. */ @@ -99,18 +97,16 @@ private TIntList removeRepeatedStops(List 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()); } @@ -125,11 +121,11 @@ private TIntList removeRepeatedStops(List 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 stopTimes) { @@ -139,10 +135,10 @@ private boolean filterStopTimes(List 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); } @@ -150,20 +146,21 @@ private boolean filterStopTimes(List stopTimes) { 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; } @@ -175,7 +172,7 @@ private boolean filterStopTimes(List 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; } @@ -190,7 +187,7 @@ private boolean filterStopTimes(List 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 @@ -229,19 +226,18 @@ private boolean filterStopTimes(List stopTimes) { */ private void interpolateStopTimes(List 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()) ) { From 7a763552939a5026836eabfe14dc6e4e9bbdaa8d Mon Sep 17 00:00:00 2001 From: Hannes Junnila Date: Thu, 18 Aug 2022 14:05:11 +0300 Subject: [PATCH 4/4] Add ValidateAndInterpolateStopTimesForEachTrip to NetexModule --- .../opentripplanner/graph_builder/module/GtfsModule.java | 2 +- .../ValidateAndInterpolateStopTimesForEachTrip.java | 9 +++++++-- src/main/java/org/opentripplanner/netex/NetexModule.java | 8 ++++++++ .../org/opentripplanner/gtfs/GtfsContextBuilder.java | 1 + 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java b/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java index 4221813d26c..fc26130e328 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java @@ -204,7 +204,7 @@ private void validateAndInterpolateStopTimesForEachTrip( TripStopTimes stopTimesByTrip, DataImportIssueStore issueStore ) { - new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, issueStore).run(); + new ValidateAndInterpolateStopTimesForEachTrip(stopTimesByTrip, true, issueStore).run(); } /** diff --git a/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java b/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java index 501c3deecd7..8467ca112e8 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/ValidateAndInterpolateStopTimesForEachTrip.java @@ -36,14 +36,16 @@ public class ValidateAndInterpolateStopTimesForEachTrip { ); private final TripStopTimes stopTimesByTrip; - + private final boolean interpolate; private final DataImportIssueStore issueStore; public ValidateAndInterpolateStopTimesForEachTrip( TripStopTimes stopTimesByTrip, + boolean interpolate, DataImportIssueStore issueStore ) { this.stopTimesByTrip = stopTimesByTrip; + this.interpolate = interpolate; this.issueStore = issueStore; } @@ -69,9 +71,12 @@ public void run() { } 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 diff --git a/src/main/java/org/opentripplanner/netex/NetexModule.java b/src/main/java/org/opentripplanner/netex/NetexModule.java index b4da7195940..9d3ee9f9bd7 100644 --- a/src/main/java/org/opentripplanner/netex/NetexModule.java +++ b/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); diff --git a/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java b/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java index d3a9b428065..9bf577f13a3 100644 --- a/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java +++ b/src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java @@ -135,6 +135,7 @@ private static GtfsImport gtfsImport(String defaultFeedId, String path) throws I private void repairStopTimesForEachTrip() { new ValidateAndInterpolateStopTimesForEachTrip( transitBuilder.getStopTimesSortedByTrip(), + true, issueStore ) .run();