diff --git a/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java b/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java index daad437e1..10d326656 100644 --- a/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java +++ b/src/main/java/com/conveyal/analysis/models/AnalysisRequest.java @@ -28,9 +28,6 @@ * sends/forwards to R5 workers (see {@link AnalysisWorkerTask}), though it has many of the same fields. */ public class AnalysisRequest { - private static int MIN_ZOOM = 9; - private static int MAX_ZOOM = 12; - private static int MAX_GRID_CELLS = 5_000_000; /** * These three IDs are redundant, and just help reduce the number of database lookups necessary. @@ -207,7 +204,6 @@ public void populateTask (AnalysisWorkerTask task, UserPermissions userPermissio // TODO define class with static factory function WebMercatorGridBounds.fromLatLonBounds(). // Also include getIndex(x, y), getX(index), getY(index), totalTasks() WebMercatorExtents extents = WebMercatorExtents.forWgsEnvelope(bounds.envelope(), zoom); - checkGridSize(extents); task.height = extents.height; task.north = extents.north; task.west = extents.west; @@ -271,21 +267,6 @@ public void populateTask (AnalysisWorkerTask task, UserPermissions userPermissio } } - private static void checkGridSize (WebMercatorExtents extents) { - if (extents.zoom < MIN_ZOOM || extents.zoom > MAX_ZOOM) { - throw AnalysisServerException.badRequest(String.format( - "Requested zoom (%s) is outside valid range (%s - %s)", extents.zoom, MIN_ZOOM, MAX_ZOOM - )); - } - if (extents.height * extents.width > MAX_GRID_CELLS) { - throw AnalysisServerException.badRequest(String.format( - "Requested number of destinations (%s) exceeds limit (%s). " + - "Set smaller custom geographic bounds or a lower zoom level.", - extents.height * extents.width, MAX_GRID_CELLS - )); - } - } - private EnumSet getEnumSetFromString (String s) { if (s != null && !"".equals(s)) { return EnumSet.copyOf(Arrays.stream(s.split(",")).map(LegMode::valueOf).collect(Collectors.toList())); diff --git a/src/main/java/com/conveyal/gtfs/model/Pattern.java b/src/main/java/com/conveyal/gtfs/model/Pattern.java index 3cd86456c..c2eeae69f 100644 --- a/src/main/java/com/conveyal/gtfs/model/Pattern.java +++ b/src/main/java/com/conveyal/gtfs/model/Pattern.java @@ -24,9 +24,15 @@ public class Pattern implements Serializable { // TODO: add set of shapes // public Set associatedShapes; - // This is the only place in the library we are still using the old JTS package name. These objects are - // serialized into MapDB files. We want to read and write MapDB files that old workers can understand. + // This is the only place we are still using the old JTS package name. + // Hopefully we can get rid of this - it's the only thing still using JTS objects under the obsolete vividsolutions + // package name so is pulling in extra dependencies and requiring conversions (toLegacyLineString). + // Unfortunately these objects are serialized into MapDB files, and we want to read and write MapDB files that + // old workers can understand. This cannot be migrated to newer JTS package names without deprecating all older + // workers, then deleting all MapDB representations of GTFS data from S3 and the local cache directory, forcing + // them to all be rebuilt the next time they're used. public com.vividsolutions.jts.geom.LineString geometry; + public String name; public String route_id; public static Joiner joiner = Joiner.on("-").skipNulls(); diff --git a/src/main/java/com/conveyal/r5/analyst/Grid.java b/src/main/java/com/conveyal/r5/analyst/Grid.java index 7a11f595e..60bb526c6 100644 --- a/src/main/java/com/conveyal/r5/analyst/Grid.java +++ b/src/main/java/com/conveyal/r5/analyst/Grid.java @@ -107,6 +107,11 @@ public Grid (int west, int north, int width, int height, int zoom) { this(new WebMercatorExtents(west, north, width, height, zoom)); } + /** + * Other constructors and factory methods all call this one, and Grid has a WebMercatorExtents field, which means + * Grid construction always follows construction of a WebMercatorExtents, whose constructor always performs a + * check on the size of the grid. + */ public Grid (WebMercatorExtents extents) { this.extents = extents; this.grid = new double[extents.width][extents.height]; diff --git a/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java b/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java index 150043b91..0d75a79ca 100644 --- a/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java +++ b/src/main/java/com/conveyal/r5/analyst/WebMercatorExtents.java @@ -1,5 +1,6 @@ package com.conveyal.r5.analyst; +import com.conveyal.analysis.AnalysisServerException; import com.conveyal.r5.analyst.cluster.AnalysisWorkerTask; import org.geotools.geometry.jts.ReferencedEnvelope; import org.geotools.referencing.CRS; @@ -17,8 +18,8 @@ import static com.google.common.base.Preconditions.checkState; /** - * Really we should be embedding one of these in the tasks, grids, etc. to factor out all the common fields. - * Equals and hashcode are semantic, for use as or within hashtable keys. + * Really we should have a field pointing to an instance of this in tasks, grids, etc. to factor out all the common + * fields. Equals and hashcode are semantic, for use as or within hashtable keys. * * TODO may want to distinguish between WebMercatorExtents, WebMercatorGrid (adds lat/lon conversion functions), * and OpportunityGrid (AKA Grid) which adds opportunity counts. These can compose, not necessarily subclass. @@ -26,6 +27,10 @@ */ public class WebMercatorExtents { + private static final int MIN_ZOOM = 9; + private static final int MAX_ZOOM = 12; + private static final int MAX_GRID_CELLS = 5_000_000; + /** The pixel number of the westernmost pixel (smallest x value). */ public final int west; @@ -44,12 +49,17 @@ public class WebMercatorExtents { /** Web mercator zoom level. */ public final int zoom; + /** + * All factory methods or constructors for WebMercatorExtents should eventually call this constructor, + * as it will enforce size constraints that prevent straining or stalling the system. + */ public WebMercatorExtents (int west, int north, int width, int height, int zoom) { this.west = west; this.north = north; this.width = width; this.height = height; this.zoom = zoom; + checkGridSize(); } /** @@ -84,7 +94,11 @@ public static WebMercatorExtents forPointsets (PointSet[] pointSets) { } } - /** Create the minimum-size immutable WebMercatorExtents containing both this one and the other one. */ + /** + * Create the minimum-size immutable WebMercatorExtents containing both this one and the other one. + * Note that WebMercatorExtents fields are immutable, and this method does not modify the instance in place. + * It creates a new instance. This behavior differs from GeoTools / JTS Envelopes. + */ public WebMercatorExtents expandToInclude (WebMercatorExtents other) { checkState(this.zoom == other.zoom, "All grids supplied must be at the same zoom level."); final int thisEast = this.west + this.width; @@ -207,4 +221,34 @@ public static Coordinate mercatorPixelToMeters (double xPixel, double yPixel, in return new Coordinate(xMeters, yMeters); } + /** + * Users may create very large grids in various ways. For example, by setting large custom analysis bounds or by + * uploading spatial data sets with very large extents. This method checks some limits on the zoom level and total + * number of cells to avoid straining or stalling the system. + * + * The fields of WebMercatorExtents are immutable and are not typically deserialized from incoming HTTP API + * requests. As all instances are created through a constructor, so we can perform this check every time a grid is + * created. If we do eventually deserialize these from HTTP API requests, we'll have to call the check explicitly. + * The Grid class internally uses a WebMercatorExtents field, so dimensions are also certain to be checked while + * constructing a Grid. + * + * An analysis destination grid might become problematic at a smaller size than an opportunity data grid. But until + * we have a reason to distinguish different situations, MAX_GRID_CELLS is defined as a constant in this class. + * If needed, we can make the max size a method parameter and pass in different limits in different contexts. + */ + public void checkGridSize () { + if (this.zoom < MIN_ZOOM || this.zoom > MAX_ZOOM) { + throw AnalysisServerException.badRequest(String.format( + "Requested zoom (%s) is outside valid range (%s - %s)", this.zoom, MIN_ZOOM, MAX_ZOOM + )); + } + if (this.height * this.width > MAX_GRID_CELLS) { + throw AnalysisServerException.badRequest(String.format( + "Requested number of destinations (%s) exceeds limit (%s). " + + "Set smaller custom geographic bounds or a lower zoom level.", + this.height * this.width, MAX_GRID_CELLS + )); + } + } + } diff --git a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java index ee2d4ffdb..ee5e8ba35 100644 --- a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java +++ b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java @@ -11,9 +11,14 @@ import java.io.IOException; /** - * Serialize to Google encoded polyline. - * Hopefully we can get rid of this - it's the only thing still using JTS objects under the vividsolutions package name - * so is pulling in extra dependencies and requiring conversions (toLegacyLineString). + * Serialize JTS LineString to Google encoded polyline. + * + * This class is the only use of dependency com.axiomalaska:polyline-encoder, and it is only used in + * com.conveyal.r5.transitive.TransitivePattern, which is in turn only used in + * com.conveyal.r5.transitive.TransitiveNetwork, which is in turn only used in + * com.conveyal.r5.analyst.cluster.AnalysisWorker#saveTauiMetadata. + * That dependency has required maintainance on a few occasions and was hosted at a repo outside Maven Central which has + * become unavailable on a few occations. We have copied the artifact to our S3-backed Conveyal Maven repo. */ public class EncodedPolylineSerializer extends JsonSerializer { diff --git a/src/test/java/com/conveyal/r5/analyst/GridTest.java b/src/test/java/com/conveyal/r5/analyst/GridTest.java index 6798f548e..03149d1c2 100644 --- a/src/test/java/com/conveyal/r5/analyst/GridTest.java +++ b/src/test/java/com/conveyal/r5/analyst/GridTest.java @@ -26,31 +26,33 @@ public class GridTest { @Test public void testGetMercatorEnvelopeMeters() throws Exception { // Southeastern Australia - // Correct meter coordinates from http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/ - int zoom = 4; - int xTile = 14; - int yTile = 9; + // Reference coordinates in meters from http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/ + int zoom = 9; + int xTile = 465; + int yTile = 312; Grid grid = new Grid(256 * xTile, 256 * yTile, 256, 256, zoom); ReferencedEnvelope envelope = grid.getWebMercatorExtents().getMercatorEnvelopeMeters(); - assertEquals(15028131.257091936, envelope.getMinX(), 0.1); - assertEquals(-5009377.085697312, envelope.getMinY(), 0.1); - assertEquals(17532819.79994059, envelope.getMaxX(), 0.1); - assertEquals(-2504688.542848654, envelope.getMaxY(), 0.1); - - // Cutting through Paris - zoom = 5; - xTile = 16; - yTile = 11; + assertEquals(16358747.0, envelope.getMinX(), 1); + assertEquals(-4461476.0, envelope.getMinY(), 1); + assertEquals(16437019.0, envelope.getMaxX(), 1); + assertEquals(-4383205.0, envelope.getMaxY(), 1); + + // The tile east of Greenwich + // Reference coordinates in meters from http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/ + // Expect the zero edge to be more exact, others to within one meter. + zoom = 12; + xTile = 2048; + yTile = 1362; grid = new Grid(256 * xTile, 256 * yTile, 256, 256, zoom); envelope = grid.getWebMercatorExtents().getMercatorEnvelopeMeters(); - assertEquals(0, envelope.getMinX(), 0.1); - assertEquals(5009377.085697312, envelope.getMinY(), 0.1); - assertEquals(1252344.271424327, envelope.getMaxX(), 0.1); - assertEquals(6261721.357121639, envelope.getMaxY(), 0.1); + assertEquals(0.0, envelope.getMinX(), 0.01); + assertEquals(6701999.0, envelope.getMinY(), 1); + assertEquals(9784.0, envelope.getMaxX(), 1); + assertEquals(6711783.0, envelope.getMaxY(), 1); // /** -// * Make sure the Mercator projection works properly. Open the resulting file in GIS and -// * compare with http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/ +// * Uncomment to manually verify that the Mercator projection works properly. Open the resulting file in GIS +// * and compare with http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/ // */ // OutputStream outputStream = new FileOutputStream("test.tiff"); // grid.writeGeotiff(outputStream); diff --git a/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java b/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java index a4b71fd9c..431dfcfbc 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java +++ b/src/test/java/com/conveyal/r5/analyst/network/GridSinglePointTaskBuilder.java @@ -108,6 +108,10 @@ public GridSinglePointTaskBuilder maxRides(int rides) { return this; } + /** + * Even if you're not actually using the opportunity count, you should call this to set the grid extents on the + * resulting task. Otherwise it will fail checks on the grid dimensions and zoom level. + */ public GridSinglePointTaskBuilder uniformOpportunityDensity (double density) { Grid grid = gridLayout.makeUniformOpportunityDataset(density); task.destinationPointSets = new PointSet[] { grid }; diff --git a/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java b/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java index 4f762a631..15370f3e7 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java +++ b/src/test/java/com/conveyal/r5/analyst/network/RandomFrequencyPhasingTests.java @@ -52,6 +52,7 @@ public void testFilteredTripRandomization () throws Exception { .weekendMorningPeak() .setOrigin(20, 20) .monteCarloDraws(1000) + .uniformOpportunityDensity(10) .build(); TravelTimeComputer computer = new TravelTimeComputer(task, network);