diff --git a/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java b/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java index 73bbd5567..6af5dbcf7 100644 --- a/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java +++ b/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); diff --git a/src/main/java/com/conveyal/r5/analyst/TravelTimeReducer.java b/src/main/java/com/conveyal/r5/analyst/TravelTimeReducer.java index 15ce02012..213091d56 100644 --- a/src/main/java/com/conveyal/r5/analyst/TravelTimeReducer.java +++ b/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) { diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/RegionalTask.java b/src/main/java/com/conveyal/r5/analyst/cluster/RegionalTask.java index 501780549..0a444f9aa 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/RegionalTask.java +++ b/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; diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/TravelTimeSurfaceTask.java b/src/main/java/com/conveyal/r5/analyst/cluster/TravelTimeSurfaceTask.java index 734534b95..4df88fd35 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/TravelTimeSurfaceTask.java +++ b/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; } } diff --git a/src/test/java/com/conveyal/r5/analyst/network/GridLayout.java b/src/test/java/com/conveyal/r5/analyst/network/GridLayout.java index 950a6cc01..b2e1daf9d 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/GridLayout.java +++ b/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. */ diff --git a/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java b/src/test/java/com/conveyal/r5/analyst/network/GridRegionalTaskBuilder.java similarity index 76% rename from src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java rename to src/test/java/com/conveyal/r5/analyst/network/GridRegionalTaskBuilder.java index c751bfe4b..5b809b447 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java +++ b/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" }; diff --git a/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java b/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java index e66599191..5330463eb 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java +++ b/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)