use regional tasks in simpson desert tests

Addresses #907, follow up to #896.
Regional tasks already allow freeform destinations.
Removed special code path required to do this with single point tasks.
Comments added to better explain the constraints and expectations around
single point and regional tasks, and how destinations are validated.
Assertions added pending better general validation on TravelTimeReducer.
Geographic dimensions and/or number of target points should be checked.

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)