From 9b5fc96869678238cb6a458c9538c83464275413 Mon Sep 17 00:00:00 2001 From: Simon Richard Date: Sat, 31 May 2025 23:59:09 -0400 Subject: [PATCH 1/5] Handle GeoJSON locations when computing distance between stops --- .../gtfsvalidator/util/StopUtil.java | 8 ++++ .../StopTimeTravelSpeedValidator.java | 20 ++++++--- .../StopTimeTravelSpeedValidatorTest.java | 42 +++++++++++++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) 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..0e2f1c39b9 100644 --- a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java +++ b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java @@ -50,6 +50,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,7 +101,8 @@ public void validate(NoticeContainer noticeContainer) { continue; } final double maxSpeedKph = getMaxVehicleSpeedKph(route.get().routeType()); - final double[] distancesKm = findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes()); + final double[] distancesKm = + findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes(), stopTable); validateConsecutiveStops(trips, distancesKm, maxSpeedKph, noticeContainer); validateFarStops(trips, distancesKm, maxSpeedKph, noticeContainer); } @@ -154,13 +156,21 @@ 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; } 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..6cde893d36 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; @@ -326,4 +329,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))); + } } From 615ccf9ab4953e0d9e8e8b2eacb1262c82dbf76a Mon Sep 17 00:00:00 2001 From: Simon Richard Date: Fri, 20 Jun 2025 22:30:43 -0400 Subject: [PATCH 2/5] Ignore GeoJSON locations when computing distance between stops --- .../StopTimeTravelSpeedValidator.java | 82 +++++++++++-------- 1 file changed, 49 insertions(+), 33 deletions(-) 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 0e2f1c39b9..4a5d7fe429 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; @@ -103,7 +102,7 @@ public void validate(NoticeContainer noticeContainer) { final double maxSpeedKph = getMaxVehicleSpeedKph(route.get().routeType()); final double[] distancesKm = findDistancesKmBetweenStops(tripAndStopTimes.getStopTimes(), stopTable); - validateConsecutiveStops(trips, distancesKm, maxSpeedKph, noticeContainer); + validateConsecutiveStops(trips, maxSpeedKph, noticeContainer); validateFarStops(trips, distancesKm, maxSpeedKph, noticeContainer); } } @@ -175,6 +174,24 @@ static double[] findDistancesKmBetweenStops( return distancesKm; } + private Optional getDistanceKm(GtfsStopTime start, GtfsStopTime end) { + if (!start.hasDepartureTime() || !end.hasArrivalTime()) { + return Optional.empty(); + } + + 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. @@ -244,40 +261,39 @@ 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(); ++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) { - 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 distanceKm = maybeDistanceKm.get(); + 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; } } From 56d34ba243532a526d6d7a44db0a07e44782d157 Mon Sep 17 00:00:00 2001 From: Simon Richard Date: Mon, 7 Jul 2025 22:40:31 -0400 Subject: [PATCH 3/5] Fix for loop condition --- .../gtfsvalidator/validator/StopTimeTravelSpeedValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 4a5d7fe429..c239f345d4 100644 --- a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java +++ b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java @@ -264,7 +264,7 @@ private void validateConsecutiveStops( List trips, double maxSpeedKph, NoticeContainer noticeContainer) { final List stopTimes = trips.get(0).getStopTimes(); GtfsStopTime start = stopTimes.get(0); - for (int i = 0; i < stopTimes.size(); ++i) { + for (int i = 0; i < stopTimes.size() - 1; ++i) { GtfsStopTime end = stopTimes.get(i + 1); Optional maybeDistanceKm = getDistanceKm(start, end); From 42462cfe6ee9c63cd1d7895c90a6cffa509c3ed5 Mon Sep 17 00:00:00 2001 From: Simon Richard Date: Thu, 17 Jul 2025 22:38:42 -0400 Subject: [PATCH 4/5] Update tests --- .../validator/StopTimeTravelSpeedValidatorTest.java | 2 ++ 1 file changed, 2 insertions(+) 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 6cde893d36..f78351bef9 100644 --- a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java +++ b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java @@ -301,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), From d7142de7fda87b5916753b6328824b14a1fdb848 Mon Sep 17 00:00:00 2001 From: Simon Richard Date: Thu, 17 Jul 2025 22:46:43 -0400 Subject: [PATCH 5/5] Move check for missing departure and arrival times --- .../validator/StopTimeTravelSpeedValidator.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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 c239f345d4..b419d31b48 100644 --- a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java +++ b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java @@ -175,10 +175,6 @@ static double[] findDistancesKmBetweenStops( } private Optional getDistanceKm(GtfsStopTime start, GtfsStopTime end) { - if (!start.hasDepartureTime() || !end.hasArrivalTime()) { - return Optional.empty(); - } - Optional maybeFirstLatLng = StopUtil.getOptionalStopOrParentLatLng(stopTable, start.stopId()); Optional maybeSecondLatLng = @@ -274,8 +270,13 @@ private void validateConsecutiveStops( if (maybeDistanceKm.isEmpty()) { continue; } - 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; + } double speedKph = getSpeedKphBetweenStops(distanceKm, start, end); if (speedKph > maxSpeedKph) {