Merge pull request #908 from conveyal/simpson-regional-task

Use regional tasks in Simpson Desert tests

src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java

@@ -88,11 +88,9 @@ public OneOriginResult computeTravelTimes() {
         ) {
             // Freeform destinations. Destination PointSet was set by handleOneRequest in the main AnalystWorker.
             destinations = request.destinationPointSets[0];
-        } else if (!isNullOrEmpty(request.destinationPointSets)) {
-            LOG.warn("ONLY VALID IN TESTING: Using PointSet object embedded in request outside regional analysis.");
-            destinations = request.destinationPointSets[0];
         } else {
             // Gridded (non-freeform) destinations. This method finds them differently for regional and single requests.
+            // We don't support freeform destinations for single point requests, as they must also return gridded travel times.
             WebMercatorExtents destinationGridExtents = request.getWebMercatorExtents();
             // Make a WebMercatorGridPointSet with the right extents, referring to the network's base grid and linkage.
             destinations = AnalysisWorkerTask.gridPointSetCache.get(destinationGridExtents, network.fullExtentGridPointSet);

src/main/java/com/conveyal/r5/analyst/TravelTimeReducer.java

@@ -346,6 +346,8 @@ public OneOriginResult finish () {
     /**
      * Sanity check: all opportunity data sets should have the same size and location as the points to which we'll
      * calculate travel times. They will only be used if we're calculating accessibility.
+     * TODO: expand to cover all cases where destinationPointSets are present, not just accessibility to Mercator grids.
+     * Need to verify first: should this be skipped for one-to-one regional requests on FreeFormPointSets?
      */
     public void checkOpportunityExtents (PointSet travelTimePointSet) {
         if (calculateAccessibility) {

src/main/java/com/conveyal/r5/analyst/cluster/RegionalTask.java

@@ -46,7 +46,8 @@ public class RegionalTask extends AnalysisWorkerTask implements Cloneable {
     public boolean oneToOne = false;
 
     /**
-     * Whether to record travel times between origins and destinations
+     * Whether to record travel times between origins and destinations. This is done automatically for
+     * TravelTimeSurfaceTask (single point tasks) but must be manually enabled on RegionalTasks using this field.
      */
     public boolean recordTimes;
 

src/main/java/com/conveyal/r5/analyst/cluster/TravelTimeSurfaceTask.java

@@ -60,22 +60,23 @@ public WebMercatorExtents getWebMercatorExtents() {
 
     @Override
     public int nTargetsPerOrigin () {
-        // In TravelTimeSurfaceTasks, the set of destinations is always determined by the web mercator extents in the
-        // request. A single WebMercatorGridPointSet is created with those extents. A single small freeform PointSet may
-        // also be present, but only in testing situations. The checkState assertions serve to verify assumptions that
-        // destinationPointSets are always set and we can polymorphically fetch the number of items for all normal and
-        // testing use cases. Once that is clearly working the assertions could be removed.
-        checkState(!isNullOrEmpty(destinationPointSets), "Expected destination PointSets to be present.");
-        checkState(destinationPointSets.length == 1, "Expected a single destination PointSet in TravelTimeSurfaceTask.");
-        PointSet destinations = destinationPointSets[0];
-        int nFeatures = destinations.featureCount();
-        if (destinations instanceof FreeFormPointSet) {
-            LOG.warn("Should currently be used only in testing: FreeFormPointSet specified in TravelTimeSurfaceTask.");
-            checkState(nFeatures == 1);
-        } else {
-            checkState(nFeatures == width * height);
+        // In TravelTimeSurfaceTasks (single-point or single-origin tasks), the set of destinations is always
+        // determined by the web mercator extents in the request itself. In many cases the destinationPointSets field
+        // is empty, but when destinationPointSets are present (when we're calculating accessibility), those PointSets
+        // are required to be gridded and exactly match the task extents (e.g. Grids wrapped in GridTransformWrapper).
+        // In normal usage this requirement is validated by TravelTimeReducer#checkOpportunityExtents but that function
+        // only checks when we're computing accessibility, and is only called when destinations are gridded. The
+        // assertions here serve to further verify this requirement, as we've seen it violated before in tests.
+        final int nTargets = width * height;
+        if (destinationPointSets != null) {
+            for (PointSet pointSet : destinationPointSets) {
+                checkState(
+                        pointSet.featureCount() == nTargets,
+                        "Destination PointSet expected to exactly match extents embedded in single point task."
+                );
+            }
         }
-        return nFeatures;
+        return nTargets;
     }
 
 }

src/test/java/com/conveyal/r5/analyst/network/GridLayout.java

@@ -2,9 +2,6 @@
 
 import com.conveyal.gtfs.GTFSFeed;
 import com.conveyal.osmlib.OSM;
-import com.conveyal.r5.analyst.Grid;
-import com.conveyal.r5.analyst.WebMercatorGridPointSet;
-import com.conveyal.r5.analyst.cluster.AnalysisWorkerTask;
 import com.conveyal.r5.common.SphericalDistanceLibrary;
 import com.conveyal.r5.profile.StreetMode;
 import com.conveyal.r5.transit.TransportNetwork;
@@ -13,13 +10,10 @@
 import org.locationtech.jts.geom.Envelope;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 import java.util.UUID;
 import java.util.stream.Stream;
 
-import static com.conveyal.r5.analyst.WebMercatorExtents.DEFAULT_ZOOM;
-
 /**
  * This is used in testing, to represent and create gridded transport systems with very regular spacing of roads and
  * transit stops, yielding highly predictable travel times that can be tested against actual output from the router.
@@ -165,8 +159,8 @@ public void addVerticalFrequencyRoute (int col, int headwayMinutes) {
     }
 
     /** Creates a builder for analysis worker tasks, which represent searches on this grid network. */
-    public GridSinglePointTaskBuilder newTaskBuilder() {
-        return new GridSinglePointTaskBuilder(this);
+    public GridRegionalTaskBuilder newTaskBuilder() {
+        return new GridRegionalTaskBuilder(this);
     }
 
     /** Get the minimum envelope containing all the points in this grid. */

src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java → src/test/java/com/conveyal/r5/analyst/network/GridRegionalTaskBuilder.java

@@ -1,11 +1,10 @@
 package com.conveyal.r5.analyst.network;
 
 import com.conveyal.r5.analyst.FreeFormPointSet;
-import com.conveyal.r5.analyst.Grid;
 import com.conveyal.r5.analyst.PointSet;
 import com.conveyal.r5.analyst.WebMercatorExtents;
 import com.conveyal.r5.analyst.cluster.AnalysisWorkerTask;
-import com.conveyal.r5.analyst.cluster.TravelTimeSurfaceTask;
+import com.conveyal.r5.analyst.cluster.RegionalTask;
 import com.conveyal.r5.analyst.decay.StepDecayFunction;
 import com.conveyal.r5.api.util.LegMode;
 import com.conveyal.r5.api.util.TransitModes;
@@ -20,21 +19,25 @@
 import static com.conveyal.r5.analyst.network.GridGtfsGenerator.WEEKEND_DATE;
 
 /**
- * This creates a task for use in tests. It uses a builder pattern but for a non-immutable task object.
- * It provides convenience methods to set all the necessary fields. This builder may be reused to produce
- * several tasks in a row with different settings, but only use the most recently produced one at any time.
- * See build() for further explanation.
+ * This creates a task for use in tests. It uses a builder pattern but modifies a non-immutable task object. It
+ * provides convenience methods to set all the necessary fields. This builder may be reused to produce several tasks in
+ * a row with different settings, but only use the most recently produced one at any time. See build() for further
+ * explanation. We want to use a limited number of destinations at exact points instead of Mercator gridded
+ * destinations, which would not be exactly aligned with the desert grid. Therefore we create regional tasks rather
+ * than single-point TravelTimeSurfaceTasks because single-point tasks always have gridded destinations (they always
+ * return gridded travel times which must be exactly aligned with any accessibility destinations).
  */
-public class GridSinglePointTaskBuilder {
+public class GridRegionalTaskBuilder {
 
     public static final int DEFAULT_MONTE_CARLO_DRAWS = 4800; // 40 per minute over a two hour window.
     private final GridLayout gridLayout;
-    private final AnalysisWorkerTask task;
 
-    public GridSinglePointTaskBuilder (GridLayout gridLayout) {
+    private final RegionalTask task;
+
+    public GridRegionalTaskBuilder(GridLayout gridLayout) {
         this.gridLayout = gridLayout;
         // We will accumulate settings into this task.
-        task = new TravelTimeSurfaceTask();
+        task = new RegionalTask();
         task.date = WEEKDAY_DATE;
         // Set defaults that can be overridden by calling builder methods.
         task.accessModes = EnumSet.of(LegMode.WALK);
@@ -52,10 +55,11 @@ public GridSinglePointTaskBuilder (GridLayout gridLayout) {
         task.monteCarloDraws = DEFAULT_MONTE_CARLO_DRAWS;
         // By default, traverse one block in a round predictable number of seconds.
         task.walkSpeed = gridLayout.streetGridSpacingMeters / gridLayout.walkBlockTraversalTimeSeconds;
+        // Unlike single point tasks, travel time recording must be enabled manually on regional tasks.
+        task.recordTimes = true;
         // Record more detailed information to allow comparison to theoretical travel time distributions.
         task.recordTravelTimeHistograms = true;
-        // Set the destination grid extents on the task, otherwise if no freeform PointSet is specified, the task will fail
-        // checks on the grid dimensions and zoom level.
+        // Set the grid extents on the task, otherwise the task will fail checks on the grid dimensions and zoom level.
         WebMercatorExtents extents = WebMercatorExtents.forWgsEnvelope(gridLayout.gridEnvelope(), DEFAULT_ZOOM);
         task.zoom = extents.zoom;
         task.north = extents.north;
@@ -64,38 +68,38 @@ public GridSinglePointTaskBuilder (GridLayout gridLayout) {
         task.height = extents.height;
     }
 
-    public GridSinglePointTaskBuilder setOrigin (int gridX, int gridY) {
+    public GridRegionalTaskBuilder setOrigin (int gridX, int gridY) {
         Coordinate origin = gridLayout.getIntersectionLatLon(gridX, gridY);
         task.fromLat = origin.y;
         task.fromLon = origin.x;
         return this;
     }
 
-    public GridSinglePointTaskBuilder weekdayMorningPeak () {
+    public GridRegionalTaskBuilder weekdayMorningPeak () {
         task.date = WEEKDAY_DATE;
         morningPeak();
         return this;
     }
 
-    public GridSinglePointTaskBuilder weekendMorningPeak () {
+    public GridRegionalTaskBuilder weekendMorningPeak () {
         task.date = WEEKEND_DATE;
         morningPeak();
         return this;
     }
 
-    public GridSinglePointTaskBuilder morningPeak () {
+    public GridRegionalTaskBuilder morningPeak () {
         task.fromTime = LocalTime.of(7, 00).toSecondOfDay();
         task.toTime = LocalTime.of(9, 00).toSecondOfDay();
         return this;
     }
 
-    public GridSinglePointTaskBuilder departureTimeWindow(int startHour, int startMinute, int durationMinutes) {
+    public GridRegionalTaskBuilder departureTimeWindow(int startHour, int startMinute, int durationMinutes) {
         task.fromTime = LocalTime.of(startHour, startMinute).toSecondOfDay();
         task.toTime = LocalTime.of(startHour, startMinute + durationMinutes).toSecondOfDay();
         return this;
     }
 
-    public GridSinglePointTaskBuilder maxRides(int rides) {
+    public GridRegionalTaskBuilder maxRides(int rides) {
         task.maxRides = rides;
         return this;
     }
@@ -105,7 +109,7 @@ public GridSinglePointTaskBuilder maxRides(int rides) {
      * Increasing the number of draws will yield a better approximation of the true travel time distribution
      * (while making the tests run slower).
      */
-    public GridSinglePointTaskBuilder monteCarloDraws (int draws) {
+    public GridRegionalTaskBuilder monteCarloDraws (int draws) {
         task.monteCarloDraws = draws;
         return this;
     }
@@ -120,7 +124,7 @@ public GridSinglePointTaskBuilder monteCarloDraws (int draws) {
      * web Mercator grid pixels. Using a single measurement point also greatly reduces the amount of travel time
      * histograms that must be computed and retained, improving the memory and run time cost of tests.
      */
-    public GridSinglePointTaskBuilder singleFreeformDestination(int x, int y) {
+    public GridRegionalTaskBuilder singleFreeformDestination(int x, int y) {
         FreeFormPointSet ps = new FreeFormPointSet(gridLayout.getIntersectionLatLon(x, y));
         // Downstream code expects to see the same number of keys and PointSet objects so initialize both.
         task.destinationPointSetKeys = new String[] { "POINT_SET" };

src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java

@@ -23,11 +23,11 @@
  * version to the next of R5 (a form of snapshot testing) this checks that they match theoretically expected travel
  * times given headways, transfer points, distances, common trunks and competing lines, etc.
  *
- * Originally we were using web Mercator gridded destinations, as this was the only option in single point analyses.
+ * Originally we were using web Mercator gridded destinations, as this was the only option in single point tasks.
  * Because these tests record travel time distributions at the destinations using a large number of Monte Carlo draws,
  * this was doing a lot of work and storing a lot of data for up to thousands of destinations we weren't actually using.
- * A testing code path now exists to measure travel times to one or more freeform destinations in single point mode.
- * However, it is still possible to measure times to the whole grid if singleFreeformDestination is not called.
+ * Regional tasks with freeform destinations are now used to measure travel times to a limited number of points.
+ * This could be extended to use gridded destination PointSets for future tests.
  */
 public class SimpsonDesertTests {
 
@@ -64,8 +64,7 @@ public void testGridScheduled () throws Exception {
         // always 10 minutes. So scheduled range is expected to be 1 minute slack, 0-20 minutes wait, 10 minutes ride,
         // 10 minutes wait, 10 minutes ride, giving 31 to 51 minutes.
         // This estimation logic could be better codified as something like TravelTimeEstimate.waitWithHeadaway(20) etc.
-
-        // TODO For some reason observed is off by 1 minute, figure out why.
+        // TODO: For some reason observed is dealyed by 1 minute. Figure out why, perhaps due to integer minute binning.
         Distribution expected = new Distribution(31, 20).delay(1);
         expected.multiAssertSimilar(oneOriginResult.travelTimes, 0);
     }
@@ -179,7 +178,7 @@ public void testOvertakingCases () throws  Exception {
         TransportNetwork network = gridLayout.generateNetwork();
 
         // 0. Reuse this task builder to produce several tasks. See caveats on build() method.
-        GridSinglePointTaskBuilder taskBuilder = gridLayout.newTaskBuilder()
+        GridRegionalTaskBuilder taskBuilder = gridLayout.newTaskBuilder()
                 .departureTimeWindow(7, 0, 5)
                 .maxRides(1)
                 .setOrigin(30, 50)