diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index 8fa452d46..02fc5075d 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -36,6 +36,7 @@ public enum NewGTFSErrorType { TABLE_TOO_LONG(Priority.MEDIUM, "Table is too long to record line numbers with a 32-bit integer, overflow will occur."), TIME_ZONE_FORMAT(Priority.MEDIUM, "Time zone format should be X."), REQUIRED_TABLE_EMPTY(Priority.MEDIUM, "This table is required by the GTFS specification but is empty."), + FEED_TRAVEL_TIMES_ROUNDED(Priority.LOW, "All travel times in the feed are rounded to the minute, which may cause unexpected results in routing applications where travel times are zero."), ROUTE_DESCRIPTION_SAME_AS_NAME(Priority.LOW, "The description of a route is identical to its name, so does not add any information."), ROUTE_LONG_NAME_CONTAINS_SHORT_NAME(Priority.LOW, "The long name of a route should complement the short name, not include it."), ROUTE_SHORT_AND_LONG_NAME_MISSING(Priority.MEDIUM, "A route has neither a long nor a short name."), diff --git a/src/main/java/com/conveyal/gtfs/error/SQLErrorStorage.java b/src/main/java/com/conveyal/gtfs/error/SQLErrorStorage.java index be42d2504..eea55f3a2 100644 --- a/src/main/java/com/conveyal/gtfs/error/SQLErrorStorage.java +++ b/src/main/java/com/conveyal/gtfs/error/SQLErrorStorage.java @@ -11,6 +11,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.Map; +import java.util.Set; import static com.conveyal.gtfs.util.Util.ensureValidNamespace; @@ -86,6 +87,12 @@ public void storeError (NewGTFSError error) { } } + public void storeErrors (Set errors) { + for (NewGTFSError error : errors) { + storeError(error); + } + } + /** * Commits any outstanding error inserts and returns the error count via a SQL query. */ diff --git a/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java index 72c22fdf1..46bc39d14 100644 --- a/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java @@ -1,14 +1,16 @@ package com.conveyal.gtfs.validator; +import com.conveyal.gtfs.error.NewGTFSError; import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.model.Route; -import com.conveyal.gtfs.model.ShapePoint; import com.conveyal.gtfs.model.Stop; import com.conveyal.gtfs.model.StopTime; import com.conveyal.gtfs.model.Trip; +import java.util.HashSet; import java.util.List; +import java.util.Set; import static com.conveyal.gtfs.error.NewGTFSErrorType.*; import static com.conveyal.gtfs.util.Util.fastDistance; @@ -20,6 +22,8 @@ public class SpeedTripValidator extends TripValidator { public static final double MIN_SPEED_KPH = 0.5; + private boolean allTravelTimesAreRounded = true; + private Set travelTimeZeroErrors = new HashSet<>(); public SpeedTripValidator(Feed feed, SQLErrorStorage errorStorage) { super(feed, errorStorage); @@ -53,9 +57,14 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< if (currStopTime.departure_time < currStopTime.arrival_time) { registerError(currStopTime, DEPARTURE_BEFORE_ARRIVAL); } - // TODO detect the case where all travel times are rounded off to minutes (i.e. tt % 60 == 0) - // In that case determine the maximum and minimum possible speed by adding/removing one minute of slack. + // Detect if travel times are rounded off to minutes. + boolean bothTravelTimesRounded = areTravelTimesRounded(prevStopTime); double travelTimeSeconds = currStopTime.arrival_time - prevStopTime.departure_time; + // If travel times are rounded and travel time is zero, determine the maximum and minimum possible speed + // by adding/removing one minute of slack. + if (bothTravelTimesRounded && travelTimeSeconds == 0) { + travelTimeSeconds += 60; + } if (checkDistanceAndTime(distanceMeters, travelTimeSeconds, currStopTime)) { // If distance and time are OK, we've got valid numbers to calculate a travel speed. double kph = (distanceMeters / 1000D) / (travelTimeSeconds / 60D / 60D); @@ -73,6 +82,26 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< } } + /** + * Completing this feed validator means checking if there were any unrounded travel times in the feed and (if so) + * registering any zero travel time errors that were passed over before the first unrounded travel time was + * encountered. If in fact all travel times are rounded to the minute, store a special feed-wide error in this case. + */ + public void complete (ValidationResult validationResult) { + if (!allTravelTimesAreRounded) storeErrors(travelTimeZeroErrors); + else registerError(NewGTFSError.forFeed(FEED_TRAVEL_TIMES_ROUNDED, null)); + } + + /** + * Check that arrival and departure time for a stop time are rounded to the minute and update + * {@link #allTravelTimesAreRounded} accordingly. + */ + private boolean areTravelTimesRounded(StopTime stopTime) { + boolean bothTravelTimesAreRounded = stopTime.departure_time % 60 == 0 && stopTime.arrival_time % 60 == 0; + if (!bothTravelTimesAreRounded) this.allTravelTimesAreRounded = false; + return bothTravelTimesAreRounded; + } + /** * This just pulls some of the range checking logic out of the main trip checking loop so it's more readable. * @return true if all values are OK @@ -88,7 +117,10 @@ private boolean checkDistanceAndTime (double distanceMeters, double travelTimeSe registerError(stopTime, TRAVEL_TIME_NEGATIVE, travelTimeSeconds); good = false; } else if (travelTimeSeconds == 0) { - registerError(stopTime, TRAVEL_TIME_ZERO); + // Only register the travel time zero error if not all travel times are rounded. Otherwise, hold onto the + // error in the travelTimeZeroErrors collection until the completion of this validator. + if (!allTravelTimesAreRounded) registerError(stopTime, TRAVEL_TIME_ZERO); + else travelTimeZeroErrors.add(createUnregisteredError(stopTime, TRAVEL_TIME_ZERO)); good = false; } return good; @@ -97,7 +129,7 @@ private boolean checkDistanceAndTime (double distanceMeters, double travelTimeSe /** * @return max speed in km/hour. */ - private double getMaxSpeedKph (Route route) { + private static double getMaxSpeedKph (Route route) { int type = -1; if (route != null) type = route.route_type; switch (type) { diff --git a/src/main/java/com/conveyal/gtfs/validator/TripValidator.java b/src/main/java/com/conveyal/gtfs/validator/TripValidator.java index d0164bd84..296742568 100644 --- a/src/main/java/com/conveyal/gtfs/validator/TripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/TripValidator.java @@ -1,10 +1,8 @@ package com.conveyal.gtfs.validator; -import com.conveyal.gtfs.error.NewGTFSError; import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.model.Route; -import com.conveyal.gtfs.model.ShapePoint; import com.conveyal.gtfs.model.Stop; import com.conveyal.gtfs.model.StopTime; import com.conveyal.gtfs.model.Trip; diff --git a/src/main/java/com/conveyal/gtfs/validator/Validator.java b/src/main/java/com/conveyal/gtfs/validator/Validator.java index 66d432579..1c4824539 100644 --- a/src/main/java/com/conveyal/gtfs/validator/Validator.java +++ b/src/main/java/com/conveyal/gtfs/validator/Validator.java @@ -6,9 +6,7 @@ import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.model.Entity; -import java.sql.SQLException; -import java.util.ArrayList; -import java.util.List; +import java.util.Set; /** * A Validator examines a whole GTFS feed or a single trip within a GTFS feed. It accumulates error messages for @@ -39,6 +37,22 @@ public void registerError(Entity entity, NewGTFSErrorType errorType) { errorStorage.storeError(NewGTFSError.forEntity(entity, errorType)); } + /** + * Stores a set of errors. + */ + public void storeErrors(Set errors) { + errorStorage.storeErrors(errors); + } + + /** + * WARNING: this method creates but DOES NOT STORE a new GTFS error. It should only be used in cases where a + * collection of errors need to be temporarily held before storing in batch (e.g., waiting to store travel time zero + * errors before it is determined that the entire feed uses travel times rounded to the minute). + */ + NewGTFSError createUnregisteredError (Entity entity, NewGTFSErrorType errorType) { + return NewGTFSError.forEntity(entity, errorType); + } + // /** // * Store an error that affects a single line of a single table. Add a single key-value pair to it. Wraps the // * underlying error constructor. diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors.json index c876d26c3..274b5b3f3 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchErrors.json @@ -18,11 +18,19 @@ "error_type" : "ROUTE_LONG_NAME_CONTAINS_SHORT_NAME", "line_number" : 2 }, { - "bad_value" : "20170916", + "bad_value" : null, "entity_id" : null, "entity_sequence" : null, "entity_type" : null, "error_id" : 2, + "error_type" : "FEED_TRAVEL_TIMES_ROUNDED", + "line_number" : null + }, { + "bad_value" : "20170916", + "entity_id" : null, + "entity_sequence" : null, + "entity_type" : null, + "error_id" : 3, "error_type" : "DATE_NO_SERVICE", "line_number" : null } ], diff --git a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts.json b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts.json index 3e02c3026..dcca06a39 100644 --- a/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts.json +++ b/src/test/resources/snapshots/com/conveyal/gtfs/graphql/GTFSGraphQLTest/canFetchFeedRowCounts.json @@ -6,7 +6,7 @@ "agency" : 1, "calendar" : 1, "calendar_dates" : 1, - "errors" : 3, + "errors" : 4, "routes" : 1, "stop_times" : 4, "stops" : 2,