diff --git a/main/src/main/java/org/mobilitydata/gtfsvalidator/util/StopUtil.java b/main/src/main/java/org/mobilitydata/gtfsvalidator/util/StopUtil.java index edcbe6688b..2f6634b711 100644 --- a/main/src/main/java/org/mobilitydata/gtfsvalidator/util/StopUtil.java +++ b/main/src/main/java/org/mobilitydata/gtfsvalidator/util/StopUtil.java @@ -32,6 +32,8 @@ public class StopUtil { * *

Returns (0, 0) if no coordinates are found in parent chain. */ + // TODO: Replace with `getOptionalStopOrParentLatLng` everywhere. Falling back to + // `S2LatLng.CENTER` can (and does) cause bugs. public static S2LatLng getStopOrParentLatLng(GtfsStopTableContainer stopTable, String stopId) { // Do not do an infinite loop since there may be a data bug and an infinite cycle of parents. for (int i = 0; i < 3; ++i) { @@ -52,6 +54,12 @@ public static S2LatLng getStopOrParentLatLng(GtfsStopTableContainer stopTable, S return S2LatLng.CENTER; } + public static Optional getOptionalStopOrParentLatLng( + GtfsStopTableContainer stopTable, String stopId) { + return Optional.of(getStopOrParentLatLng(stopTable, stopId)) + .filter(s2LatLng -> s2LatLng != S2LatLng.CENTER); + } + /** * Finds station that the given location belongs to. * diff --git a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java index 10694753a1..b419d31b48 100644 --- a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java +++ b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java @@ -26,8 +26,7 @@ import com.google.common.hash.HashFunction; import com.google.common.hash.Hasher; import com.google.common.hash.Hashing; -import java.util.List; -import java.util.Optional; +import java.util.*; import javax.inject.Inject; import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice; import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice.FileRefs; @@ -50,6 +49,7 @@ import org.mobilitydata.gtfsvalidator.table.GtfsTripTableContainer; import org.mobilitydata.gtfsvalidator.type.GtfsTime; import org.mobilitydata.gtfsvalidator.util.S2Earth; +import org.mobilitydata.gtfsvalidator.util.StopUtil; /** * Validates that transit vehicles do not travel too fast between consecutive and between far stops. @@ -100,8 +100,9 @@ public void validate(NoticeContainer noticeContainer) { continue; } final double maxSpeedKph = getMaxVehicleSpeedKph(route.get().routeType()); - final double[] distancesKm = findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes()); - validateConsecutiveStops(trips, distancesKm, maxSpeedKph, noticeContainer); + final double[] distancesKm = + findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes(), stopTable); + validateConsecutiveStops(trips, maxSpeedKph, noticeContainer); validateFarStops(trips, distancesKm, maxSpeedKph, noticeContainer); } } @@ -154,17 +155,39 @@ public long tripFprint() { *

{@code distancesKm[i]} equals to distance between stops corresponding to stop times i, i + * 1. */ - private double[] findDistancesKmBetweenStops(List stopTimes) { + @VisibleForTesting + static double[] findDistancesKmBetweenStops( + List stopTimes, GtfsStopTableContainer stopTable) { double[] distancesKm = new double[stopTimes.size() - 1]; S2LatLng currLatLng = getStopOrParentLatLng(stopTable, stopTimes.get(0).stopId()); for (int i = 0; i < distancesKm.length; ++i) { - S2LatLng nextLatLng = getStopOrParentLatLng(stopTable, stopTimes.get(i + 1).stopId()); - distancesKm[i] = S2Earth.getDistanceKm(currLatLng, nextLatLng); - currLatLng = nextLatLng; + Optional maybeNextLatLng = + StopUtil.getOptionalStopOrParentLatLng(stopTable, stopTimes.get(i + 1).stopId()); + if (maybeNextLatLng.isPresent()) { + S2LatLng nextLatLng = maybeNextLatLng.get(); + distancesKm[i] = S2Earth.getDistanceKm(currLatLng, nextLatLng); + currLatLng = nextLatLng; + } else { + distancesKm[i] = 0; + } } return distancesKm; } + private Optional getDistanceKm(GtfsStopTime start, GtfsStopTime end) { + Optional maybeFirstLatLng = + StopUtil.getOptionalStopOrParentLatLng(stopTable, start.stopId()); + Optional maybeSecondLatLng = + StopUtil.getOptionalStopOrParentLatLng(stopTable, end.stopId()); + + if (maybeFirstLatLng.isEmpty() || maybeSecondLatLng.isEmpty()) { + return Optional.empty(); + } + + double distanceKm = S2Earth.getDistanceKm(maybeFirstLatLng.get(), maybeSecondLatLng.get()); + return Optional.of(distanceKm); + } + /** * Validates travel speed between far stops for all trips that belong to the same route and visit * the same stops at the same times. @@ -234,40 +257,44 @@ private void validateFarStops( *

If there is a fast travel detected, then a separate notice is issued for each trip. */ private void validateConsecutiveStops( - List trips, - double[] distancesKm, - double maxSpeedKph, - NoticeContainer noticeContainer) { + List trips, double maxSpeedKph, NoticeContainer noticeContainer) { final List stopTimes = trips.get(0).getStopTimes(); - for (int i = 0; i < distancesKm.length; ++i) { - final GtfsStopTime stopTime1 = stopTimes.get(i); - final GtfsStopTime stopTime2 = stopTimes.get(i + 1); - if (!(stopTime1.hasDepartureTime() && stopTime2.hasArrivalTime())) { + GtfsStopTime start = stopTimes.get(0); + for (int i = 0; i < stopTimes.size() - 1; ++i) { + GtfsStopTime end = stopTimes.get(i + 1); + + Optional maybeDistanceKm = getDistanceKm(start, end); + // We couldn't calculate the distance, for instance because one of the stops is + // actually a GeoJSON location and doesn't have a specific latitude and longitude. + // We try comparing with the next stop instead. + if (maybeDistanceKm.isEmpty()) { continue; } - final double distanceKm = distancesKm[i]; - final double speedKph = getSpeedKphBetweenStops(distanceKm, stopTime1, stopTime2); - if (speedKph <= maxSpeedKph) { + double distanceKm = maybeDistanceKm.get(); + + // We can't calculate the speed if we're missing a departure time or + // arrival time. + if (!start.hasDepartureTime() || !end.hasArrivalTime()) { continue; } - final Optional stop1 = stopTable.byStopId(stopTime1.stopId()); - final Optional stop2 = stopTable.byStopId(stopTime2.stopId()); - if (stop1.isEmpty() || stop2.isEmpty()) { - // Broken reference is reported in another rule. - return; - } - // Issue one notice per each trip. - for (TripAndStopTimes trip : trips) { - noticeContainer.addValidationNotice( - new FastTravelBetweenConsecutiveStopsNotice( - trip.getTrip(), - trip.getStopTimes().get(i), - stop1.get(), - trip.getStopTimes().get(i + 1), - stop2.get(), - speedKph, - distanceKm)); + double speedKph = getSpeedKphBetweenStops(distanceKm, start, end); + + if (speedKph > maxSpeedKph) { + final Optional stop1 = stopTable.byStopId(start.stopId()); + final Optional stop2 = stopTable.byStopId(end.stopId()); + // This should always evaluate to true since we check whether both stops exist + // in `getDistanceAndSpeed`; this is just here as a precaution. + if (stop1.isPresent() && stop2.isPresent()) { + // Issue one notice for each trip. + for (TripAndStopTimes trip : trips) { + noticeContainer.addValidationNotice( + new FastTravelBetweenConsecutiveStopsNotice( + trip.getTrip(), start, stop1.get(), end, stop2.get(), speedKph, distanceKm)); + } + } } + + start = end; } } diff --git a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java index 6a9d0888bd..f78351bef9 100644 --- a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java +++ b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java @@ -17,13 +17,16 @@ package org.mobilitydata.gtfsvalidator.validator; import static com.google.common.truth.Truth.assertThat; +import static org.mobilitydata.gtfsvalidator.validator.StopTimeTravelSpeedValidator.findDistancesKmBetweenStops; import static org.mobilitydata.gtfsvalidator.validator.StopTimeTravelSpeedValidator.getSpeedKphBetweenStops; import com.google.common.collect.ImmutableList; import com.google.common.geometry.S2LatLng; +import com.ibm.icu.impl.Pair; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -298,6 +301,8 @@ public void farStopsMissingTimeInBetween_yieldsNotice() { GtfsTime.fromString("08:02:30"))); assertThat(generateNotices(ImmutableList.of(route), ImmutableList.of(trip), stopTimes, stops)) .containsExactly( + createFastTravelBetweenConsecutiveStopsNotice( + trip, stopTimes.get(0), stops.get(0), stopTimes.get(2), stops.get(2)), createFastTravelBetweenFarStopsNotice( trip, stopTimes.get(0), @@ -326,4 +331,43 @@ public void getSpeedKphBetweenStops_teleport() { createStopTimesSameDepartureArrival(ImmutableList.of(GtfsTime.fromString("08:00:00"))); assertThat(getSpeedKphBetweenStops(2, stopTimes.get(0), stopTimes.get(0))).isEqualTo(120); } + + @Test + public void findDistancesKmBetweenStops_geoJsonLocation() { + List points = + ImmutableList.of( + S2LatLng.fromDegrees(42.879, -73.19313), // s0 + S2LatLng.fromDegrees(42.89522, -73.20155)); // 21 + List stops = createStops(points); + + List>> stopIdsAndTimes = + ImmutableList.of( + Optional.of(Pair.of("s0", GtfsTime.fromString("14:48:00"))), + // There's a GeoJSON location between the two stops. + Optional.empty(), + Optional.of(Pair.of("s1", GtfsTime.fromString("14:56:00")))); + + List stopTimes = new ArrayList<>(stopIdsAndTimes.size()); + for (int i = 0; i < stopIdsAndTimes.size(); ++i) { + GtfsStopTime.Builder stopTimeBuilder = + new GtfsStopTime.Builder().setTripId(TEST_TRIP_ID).setStopSequence(i); + + stopIdsAndTimes + .get(i) + .ifPresent( + t -> + stopTimeBuilder + .setArrivalTime(t.second) + .setDepartureTime(t.second) + .setStopId(t.first)); + stopTimes.add(stopTimeBuilder.build()); + } + + double[] distancesKm = + findDistancesKmBetweenStops(stopTimes, GtfsStopTableContainer.forEntities(stops, null)); + + assertThat(distancesKm).hasLength(2); + assertThat(distancesKm[0]).isEqualTo(0.0); + assertThat(distancesKm[1]).isEqualTo(S2Earth.getDistanceKm(points.get(0), points.get(1))); + } }