Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename and clean up RepairStopTimesForEachTripOperation #4388

Merged
merged 5 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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();
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
Expand All @@ -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.
*/
Expand All @@ -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());
}
Expand All @@ -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) {
Expand All @@ -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;
}
Expand All @@ -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;
}

Expand All @@ -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
Expand Down Expand Up @@ -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())
) {
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/org/opentripplanner/netex/NetexModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}

Expand Down