Merge pull request #145 from conveyal/travel-time-zero-fix

Handle zero hops separately if all travel times in feed are rounded

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."),

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<NewGTFSError> errors) {
+        for (NewGTFSError error : errors) {
+            storeError(error);
+        }
+    }
+
     /**
      * Commits any outstanding error inserts and returns the error count via a SQL query.
      */

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<NewGTFSError> travelTimeZeroErrors = new HashSet<>();
 
     public SpeedTripValidator(Feed feed, SQLErrorStorage errorStorage) {
         super(feed, errorStorage);
@@ -53,9 +57,14 @@ public void validateTrip(Trip trip, Route route, List<StopTime> 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<StopTime> 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) {

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;

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<NewGTFSError> 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.

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
       } ],

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,