From 76d89e903abccae17462e199a595bf23222b158d Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 11 Oct 2018 17:08:08 -0400 Subject: [PATCH 1/4] fix(validation): handle zero hops separately if all travel times rounded refs #110 --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 1 + .../conveyal/gtfs/error/SQLErrorStorage.java | 7 ++++ .../gtfs/validator/SpeedTripValidator.java | 41 ++++++++++++++++--- .../gtfs/validator/TripValidator.java | 2 - .../conveyal/gtfs/validator/Validator.java | 20 +++++++-- 5 files changed, 61 insertions(+), 10 deletions(-) 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..4ade8cc5d 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,13 @@ 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 +81,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 +116,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 +128,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. From ffa2080a3357b4ca6f0b721c2ca7c40ef027b1c3 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 15:14:22 -0500 Subject: [PATCH 2/4] style: address PR comment about bracketed conditional block --- .../java/com/conveyal/gtfs/validator/SpeedTripValidator.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java index 4ade8cc5d..46bc39d14 100644 --- a/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java @@ -62,8 +62,9 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< 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) + 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); From dddc8fe0ee6124c6d2b63fe9fae92f72a96130c8 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 15:21:07 -0500 Subject: [PATCH 3/4] test: fix test snapshot to include rounded travel times error --- .../gtfs/graphql/GTFSGraphQLTest/canFetchErrors.json | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 d50b0274a..0eaafe2d9 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 @@ -17,7 +17,16 @@ "error_id" : 1, "error_type" : "ROUTE_LONG_NAME_CONTAINS_SHORT_NAME", "line_number" : 2 - } ], + }, + { + "bad_value" : null, + "entity_id" : null, + "entity_sequence" : null, + "entity_type" : null, + "error_id" : 2, + "error_type" : "FEED_TRAVEL_TIMES_ROUNDED", + "line_number" : null + } ], "feed_version" : "1.0" } } From 0e15a54cb5950f5b3bb6020bc6d18d6ba527e02c Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 18 Dec 2018 15:36:55 -0500 Subject: [PATCH 4/4] test(snapshot): fix failing error fetch snapshot (whitespace issue) --- .../graphql/GTFSGraphQLTest/canFetchErrors.json | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 553b04ced..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,13 +18,13 @@ "error_type" : "ROUTE_LONG_NAME_CONTAINS_SHORT_NAME", "line_number" : 2 }, { - "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" : 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,