Add table of per-edge factors to network when modification requires it

src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java

@@ -139,7 +139,7 @@ public boolean apply(TransportNetwork network) {
                 .collect(Collectors.toList());
 
         if (nTripsAffected > 0) {
-            info.add(String.format("Speed was changed on %d trips.", nTripsAffected));
+            info.add(String.format("Changed speed on %d trips.", nTripsAffected));
         } else {
             errors.add("This modification did not cause any changes to the transport network.");
         }

src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java

@@ -4,6 +4,7 @@
 import com.conveyal.r5.common.GeometryUtils;
 import com.conveyal.r5.profile.StreetMode;
 import com.conveyal.r5.streets.EdgeStore;
+import com.conveyal.r5.streets.EdgeTraversalTimes;
 import com.conveyal.r5.transit.TransportNetwork;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import gnu.trove.iterator.TIntIterator;
@@ -23,7 +24,27 @@
 import static com.conveyal.r5.labeling.LevelOfTrafficStressLabeler.intToLts;
 
 /**
+ * <p>
  * This modification selects all edges inside a given set of polygons and changes their characteristics.
+ * </p><p>
+ * Some of its options, specifically walkTimeFactor and bikeTimeFactor, adjust generalized costs for walking and biking
+ * which are stored in an optional generalized costs data table that is not present on networks by default.
+ * These data tables are currently only created in networks built from very particular OSM data where every way has all
+ * of the special tags contributing to the LADOT generalized costs (com.conveyal.r5.streets.LaDotCostTags).
+ * </p><p>
+ * The apply() method creates this data table in the scenario copy of the network as needed if one does not exist on the
+ * base network (so there is no extend-only wrapper in the scenario network). This means each scenario may have its own
+ * (potentially large) generalized cost data table instead of just extending a shared one in the baseline network.
+ * This less-than-optimal implementation is acceptable at least as a stopgap on this rarely used specialty modification.
+ * The other alternatives would be:
+ * </p><ul>
+ * <li> Add the table to the baseline network whenever it's any scenario needs to extend it.
+ *      This breaks a lot of conventions we have about treating loaded networks as read-only, and incurs a lot of extra
+ *      memory access and pointless multiplication by 1 on every scenario including the baseline.</li>
+ * <li> Require the table to be enabled on the base network when it's first built, using a parameter in
+ *      TransportNetworkConfig. This incurs the same overhead, but respects the immutable character of loaded networks
+ *      and is an intentional choice by the user.</li>
+ * </ul>
  */
 public class ModifyStreets extends Modification {
 
@@ -138,17 +159,11 @@ public boolean resolve (TransportNetwork network) {
             }
         }
         if (walkTimeFactor != null) {
-            if (network.streetLayer.edgeStore.edgeTraversalTimes == null && walkTimeFactor != 1) {
-                errors.add("walkGenCostFactor can only be set to values other than 1 on networks that support per-edge factors.");
-            }
             if (walkTimeFactor <= 0 || walkTimeFactor > 10) {
                 errors.add("walkGenCostFactor must be in the range (0...10].");
             }
         }
         if (bikeTimeFactor != null) {
-            if (network.streetLayer.edgeStore.edgeTraversalTimes == null && bikeTimeFactor != 1) {
-                errors.add("bikeGenCostFactor can only be set to values other than 1 on networks that support per-edge factors.");
-            }
             if (bikeTimeFactor <= 0 || bikeTimeFactor > 10) {
                 errors.add("bikeGenCostFactor must be in the range (0...10].");
             }
@@ -167,6 +182,12 @@ public boolean resolve (TransportNetwork network) {
     @Override
     public boolean apply (TransportNetwork network) {
         EdgeStore edgeStore = network.streetLayer.edgeStore;
+        if (network.streetLayer.edgeStore.edgeTraversalTimes == null) {
+            if ((walkTimeFactor != null && walkTimeFactor != 1) || (bikeTimeFactor != null && bikeTimeFactor != 1)) {
+                info.add("Added table of per-edge factors because base network doesn't have one.");
+                network.streetLayer.edgeStore.edgeTraversalTimes = EdgeTraversalTimes.createNeutral(network.streetLayer.edgeStore);
+            }
+        }
         EdgeStore.Edge oldEdge = edgeStore.getCursor();
         // By convention we only index the forward edge in each pair, so we're iterating over forward edges here.
         for (TIntIterator edgeIterator = edgesInPolygon.iterator(); edgeIterator.hasNext(); ) {

src/main/java/com/conveyal/r5/streets/EdgeStore.java

@@ -373,8 +373,8 @@ public Edge addStreetPair(int beginVertexIndex, int endVertexIndex, int edgeLeng
         flags.add(0);
 
         if (edgeTraversalTimes != null) {
-            edgeTraversalTimes.addOneEdge();
-            edgeTraversalTimes.addOneEdge();
+            edgeTraversalTimes.addOneNeutralEdge();
+            edgeTraversalTimes.addOneNeutralEdge();
         }
 
         Edge edge = getCursor(forwardEdgeIndex);

src/main/java/com/conveyal/r5/streets/EdgeTraversalTimes.java

@@ -80,10 +80,10 @@ public void copyTimes (int oldEdge, int newEdge, Double walkFactor, Double bikeF
         bikeTraversalTimes.copyTimes(oldEdge, newEdge, bikeFactor);
     }
 
-    // Stopgap to pad out the traversal times when adding new edges
-    public void addOneEdge () {
-        walkTraversalTimes.setOneEdge();
-        bikeTraversalTimes.setOneEdge();
+    /** Pad out the traversal multipliers/costs with neutral values. */
+    public void addOneNeutralEdge () {
+        walkTraversalTimes.addOneNeutralEdge();
+        bikeTraversalTimes.addOneNeutralEdge();
     }
 
     public void setWalkTimeFactor (int edgeIndex, double walkTimeFactor) {
@@ -106,4 +106,18 @@ public double getWalkTimeFactor (int edgeIndex) {
     public double getBikeTimeFactor (int edgeIndex) {
         return bikeTraversalTimes.perceivedLengthMultipliers.get(edgeIndex);
     }
+
+    /**
+     * Factory method returning a newly constructed EdgeTraversalTimes where all scaling factors are 1 and constant
+     * costs are 0. This serves as a neutral starting point for building up generalized costs in modifications,
+     * as opposed to starting from pre-existing generalized costs derived from special purpose OSM tags.
+     */
+    public static EdgeTraversalTimes createNeutral (EdgeStore edgeStore) {
+        var times = new EdgeTraversalTimes(edgeStore);
+        for (int i = 0; i < edgeStore.nEdges(); i++) {
+            times.addOneNeutralEdge();
+        }
+        return times;
+    }
+
 }

src/main/java/com/conveyal/r5/streets/SingleModeTraversalTimes.java

@@ -80,7 +80,12 @@ public interface Supplier {
         int turnTimeSeconds (TurnDirection turnDirection);
     }
 
-    public void setOneEdge () {
+
+    /**
+     * Add data for a single edge, setting scaling factors to 1 and constant costs to 0.
+     * This serves as a neutral starting point for adding generalized costs later in modifications.
+     */
+    public void addOneNeutralEdge () {
         perceivedLengthMultipliers.add(1);
         this.leftTurnSeconds.add(0);
         this.rightTurnSeconds.add(0);

src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java

@@ -0,0 +1,112 @@
+package com.conveyal.r5.analyst.scenario;
+
+import com.conveyal.r5.analyst.WebMercatorExtents;
+import com.conveyal.r5.analyst.WebMercatorGridPointSet;
+import com.conveyal.r5.analyst.cluster.TravelTimeSurfaceTask;
+import com.conveyal.r5.api.util.LegMode;
+import com.conveyal.r5.api.util.TransitModes;
+import com.conveyal.r5.profile.StreetMode;
+import com.conveyal.r5.streets.StreetRouter;
+import com.conveyal.r5.transit.TransportNetwork;
+import com.conveyal.r5.util.LambdaCounter;
+import gnu.trove.map.TIntIntMap;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.locationtech.jts.geom.Envelope;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.EnumSet;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test that Modifications affecting streets work correctly.
+ */
+public class ModifyStreetsTest {
+
+    final double FROM_LAT = 40.0;
+    final double FROM_LON = -83.0;
+    final int TIME_LIMIT_SECONDS = 1200;
+    // Analysis bounds
+    final double SIZE_DEGREES = 0.05;
+    final double WEST = FROM_LON - SIZE_DEGREES;
+    final double EAST = FROM_LON + SIZE_DEGREES;
+    final double SOUTH = FROM_LAT - SIZE_DEGREES;
+    final double NORTH = FROM_LAT + SIZE_DEGREES;
+
+    /**
+     * Using a `ModifyStreets` modification, test that adjusting the `walkTimeFactor` appropriately changes route time.
+     * Also indirectly tests that the necessary generalized cost data tables will be added to the network when missing.
+     */
+    @Test
+    public void testGeneralizedCostWalk() {
+        var network = FakeGraph.buildNetwork(FakeGraph.TransitNetwork.MULTIPLE_LINES);
+
+        var ms = new ModifyStreets();
+        ms.allowedModes = EnumSet.of(StreetMode.WALK);
+        ms.walkTimeFactor = 0.1;
+        ms.polygons = new double[][][]{{
+                {WEST, NORTH},
+                {EAST, NORTH},
+                {EAST, SOUTH},
+                {WEST, SOUTH},
+                {WEST, NORTH}
+        }};
+
+        var scenario = new Scenario();
+        scenario.modifications = Arrays.asList(ms);
+
+        var reachedVertices = getReachedVertices(network, new Scenario());
+        var reachedVerticesWithModification = getReachedVertices(network, scenario);
+
+        int nReachedVertices = reachedVertices.size();
+        int nReachedVerticesWithModification = reachedVerticesWithModification.size();
+
+        assertTrue(nReachedVertices > 500);
+        assertTrue(nReachedVerticesWithModification > nReachedVertices * 2);
+
+        int[] nLowerTimes = new int[1]; // Sidestep annoying Java "effectively final" rule.
+        reachedVertices.forEachKey(v -> {
+            int travelTime = reachedVertices.get(v);
+            int travelTimeWithModification = reachedVerticesWithModification.get(v);
+            assertTrue(travelTimeWithModification != -1);
+            assertTrue(travelTimeWithModification <= travelTime);
+            if (travelTimeWithModification != travelTime) {
+                nLowerTimes[0] += 1;
+                assertTrue(travelTimeWithModification * 1.3 < travelTime);
+            }
+            return true;
+        });
+        assertTrue(nLowerTimes[0] > 500);
+    }
+
+    TIntIntMap getReachedVertices(TransportNetwork network, Scenario scenario) {
+        var modifiedNetwork = scenario.applyToTransportNetwork(network);
+        Assertions.assertEquals(0, modifiedNetwork.scenarioApplicationWarnings.size());
+
+        var extents = WebMercatorExtents.forWgsEnvelope(new Envelope(WEST, EAST, SOUTH, NORTH), WebMercatorGridPointSet.DEFAULT_ZOOM);
+        var task = new TravelTimeSurfaceTask();
+        task.accessModes = EnumSet.of(LegMode.WALK);
+        task.directModes = EnumSet.of(LegMode.WALK);
+        task.transitModes = EnumSet.noneOf(TransitModes.class);
+        task.percentiles = new int[]{50};
+        task.fromLat = FROM_LAT;
+        task.fromLon = FROM_LON;
+        task.north = extents.north;
+        task.west = extents.west;
+        task.height = extents.height;
+        task.width = extents.width;
+        task.zoom = extents.zoom;
+
+        var streetRouter = new StreetRouter(modifiedNetwork.streetLayer);
+        streetRouter.profileRequest = task;
+        streetRouter.setOrigin(task.fromLat, task.fromLon);
+        streetRouter.streetMode = StreetMode.WALK;
+        streetRouter.timeLimitSeconds = TIME_LIMIT_SECONDS;
+        streetRouter.route();
+
+        return streetRouter.getReachedVertices();
+    }
+}