From ccc302f391968852b13477c01045e8b182dcf95b Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Wed, 25 May 2022 22:01:31 +0800 Subject: [PATCH 01/46] Zoom: add to opportunity dataset model (#807) * Store zoom level in opportunity dataset model Retrieve it from the WebMercatorExtents that are passed in on creation. * Require zoom level to be passed on creation This requires zoom levels to be specified when creating opportunity datasets. * Use `zoom` instead of `webMercatorZoom` --- .../OpportunityDatasetController.java | 9 ++------- .../RegionalAnalysisController.java | 4 +--- .../analysis/models/OpportunityDataset.java | 20 +++++++++---------- 3 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/controllers/OpportunityDatasetController.java b/src/main/java/com/conveyal/analysis/controllers/OpportunityDatasetController.java index 012309ee1..cd2932c03 100644 --- a/src/main/java/com/conveyal/analysis/controllers/OpportunityDatasetController.java +++ b/src/main/java/com/conveyal/analysis/controllers/OpportunityDatasetController.java @@ -24,7 +24,6 @@ import com.conveyal.r5.analyst.progress.NoopProgressListener; import com.conveyal.r5.analyst.progress.Task; import com.conveyal.r5.analyst.progress.WorkProduct; -import com.conveyal.r5.analyst.progress.WorkProductType; import com.conveyal.r5.util.ExceptionUtils; import com.conveyal.r5.util.InputStreamProvider; import com.conveyal.r5.util.ProgressListener; @@ -32,10 +31,6 @@ import com.google.common.io.Files; import com.mongodb.QueryBuilder; import org.apache.commons.fileupload.FileItem; -import org.apache.commons.fileupload.FileItemFactory; -import org.apache.commons.fileupload.FileUploadException; -import org.apache.commons.fileupload.disk.DiskFileItemFactory; -import org.apache.commons.fileupload.servlet.ServletFileUpload; import org.apache.commons.io.FilenameUtils; import org.bson.types.ObjectId; import org.slf4j.Logger; @@ -210,7 +205,7 @@ private void updateAndStoreDatasets (SpatialDataSource source, if (dataset.format == FileStorageFormat.FREEFORM) { dataset.name = String.join(" ", pointSet.name, "(freeform)"); } - dataset.setWebMercatorExtents(pointSet); + dataset.setWebMercatorExtents(pointSet.getWebMercatorExtents()); // TODO make origin and destination pointsets reference each other and indicate they are suitable // for one-to-one analyses @@ -327,7 +322,7 @@ private OpportunityDatasetUploadStatus createOpportunityDataset(Request req, Res // Parse required fields. Will throw a ServerException on failure. final String sourceName = HttpUtils.getFormField(formFields, "Name", true); final String regionId = HttpUtils.getFormField(formFields, "regionId", true); - final int zoom = parseZoom(HttpUtils.getFormField(formFields, "zoom", false)); + final int zoom = parseZoom(HttpUtils.getFormField(formFields, "zoom", true)); // Create a region-wide status object tracking the processing of opportunity data. // Create the status object before doing anything including input and parameter validation, so that any problems diff --git a/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java b/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java index 60197114b..32336fd7c 100644 --- a/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java +++ b/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java @@ -4,7 +4,6 @@ import com.conveyal.analysis.SelectingGridReducer; import com.conveyal.analysis.UserPermissions; import com.conveyal.analysis.components.broker.Broker; -import com.conveyal.analysis.components.broker.Job; import com.conveyal.analysis.components.broker.JobStatus; import com.conveyal.analysis.models.AnalysisRequest; import com.conveyal.analysis.models.OpportunityDataset; @@ -33,7 +32,6 @@ import java.io.File; import java.io.FileInputStream; -import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; @@ -402,7 +400,7 @@ private RegionalAnalysis createRegionalAnalysis (Request req, Response res) thro ); } else { checkArgument( - dataset.getWebMercatorExtents().zoom == opportunityDatasets.get(0).getWebMercatorExtents().zoom, + dataset.zoom == opportunityDatasets.get(0).zoom, "If multiple grids are specified as destinations, they must have identical resolutions (web mercator zoom levels)." ); } diff --git a/src/main/java/com/conveyal/analysis/models/OpportunityDataset.java b/src/main/java/com/conveyal/analysis/models/OpportunityDataset.java index 773c4efd2..ecf66134b 100644 --- a/src/main/java/com/conveyal/analysis/models/OpportunityDataset.java +++ b/src/main/java/com/conveyal/analysis/models/OpportunityDataset.java @@ -2,12 +2,10 @@ import com.conveyal.file.FileStorageFormat; import com.conveyal.file.FileStorageKey; -import com.conveyal.r5.analyst.PointSet; import com.conveyal.r5.analyst.WebMercatorExtents; import com.fasterxml.jackson.annotation.JsonIgnore; import static com.conveyal.file.FileCategory.GRIDS; -import static com.conveyal.r5.analyst.WebMercatorGridPointSet.DEFAULT_ZOOM; /** * A model object for storing metadata about opportunity datasets in Mongo, for sharing it with the frontend. @@ -34,13 +32,18 @@ public class OpportunityDataset extends Model { public String bucketName; /** - * Bounds in web Mercator pixels. Note that no zoom level is specified here, it's fixed to a constant 9. + * Bounds in web Mercator pixels. */ public int north; public int west; public int width; public int height; + /** + * The zoom level this opportunity dataset was rasterized at. + */ + public int zoom; + /** * Total number of opportunities in the dataset, i.e. the sum of all opportunity counts at all points / grid cells. * It appears the UI doesn't use this now, but it could. We might want to remove it. @@ -103,21 +106,16 @@ public FileStorageKey getStorageKey (FileStorageFormat fileFormat) { } @JsonIgnore - public WebMercatorExtents getWebMercatorExtents () { - return new WebMercatorExtents(west, north, width, height, DEFAULT_ZOOM); - } - - @JsonIgnore - public void setWebMercatorExtents (PointSet pointset) { + public void setWebMercatorExtents (WebMercatorExtents extents) { // These bounds are currently in web Mercator pixels, which are relevant to Grids but are not natural units // for FreeformPointSets. There are only unique minimal web Mercator bounds for FreeformPointSets if - // the zoom level is fixed in OpportunityDataset (FIXME we may change this soon). + // the zoom level is fixed in OpportunityDataset. // Perhaps these metadata bounds should be WGS84 instead, it depends how the UI uses them. - WebMercatorExtents extents = pointset.getWebMercatorExtents(); this.west = extents.west; this.north = extents.north; this.width = extents.width; this.height = extents.height; + this.zoom = extents.zoom; } /** Analysis region this dataset was uploaded in. */ From 9fcc782d1dfc88a3f429ee9ab2764a25cd76612a Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Mon, 13 Jun 2022 22:17:35 -0400 Subject: [PATCH 02/46] Update issue templates These are the stock templates provided by Github, they are a great start but we may want to customize them to fit our needs. --- .github/ISSUE_TEMPLATE/bug_report.md | 38 +++++++++++++++++++++++ .github/ISSUE_TEMPLATE/feature_request.md | 20 ++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/bug_report.md create mode 100644 .github/ISSUE_TEMPLATE/feature_request.md diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md new file mode 100644 index 000000000..dd84ea782 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -0,0 +1,38 @@ +--- +name: Bug report +about: Create a report to help us improve +title: '' +labels: '' +assignees: '' + +--- + +**Describe the bug** +A clear and concise description of what the bug is. + +**To Reproduce** +Steps to reproduce the behavior: +1. Go to '...' +2. Click on '....' +3. Scroll down to '....' +4. See error + +**Expected behavior** +A clear and concise description of what you expected to happen. + +**Screenshots** +If applicable, add screenshots to help explain your problem. + +**Desktop (please complete the following information):** + - OS: [e.g. iOS] + - Browser [e.g. chrome, safari] + - Version [e.g. 22] + +**Smartphone (please complete the following information):** + - Device: [e.g. iPhone6] + - OS: [e.g. iOS8.1] + - Browser [e.g. stock browser, safari] + - Version [e.g. 22] + +**Additional context** +Add any other context about the problem here. diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md new file mode 100644 index 000000000..bbcbbe7d6 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -0,0 +1,20 @@ +--- +name: Feature request +about: Suggest an idea for this project +title: '' +labels: '' +assignees: '' + +--- + +**Is your feature request related to a problem? Please describe.** +A clear and concise description of what the problem is. Ex. I'm always frustrated when [...] + +**Describe the solution you'd like** +A clear and concise description of what you want to happen. + +**Describe alternatives you've considered** +A clear and concise description of any alternative solutions or features you've considered. + +**Additional context** +Add any other context or screenshots about the feature request here. From 51120c2414c92fe95b51879da12c835feca52939 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Wed, 22 Jun 2022 08:19:49 +0800 Subject: [PATCH 03/46] Add zoom level to aggregation areas (#813) --- .../datasource/derivation/AggregationAreaDerivation.java | 5 +---- .../java/com/conveyal/analysis/models/AggregationArea.java | 5 ++++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java b/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java index 76ca0177f..dfe994291 100644 --- a/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java +++ b/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java @@ -22,8 +22,6 @@ import org.locationtech.jts.geom.Geometry; import org.locationtech.jts.operation.union.UnaryUnionOp; import org.opengis.feature.simple.SimpleFeature; -import org.opengis.referencing.FactoryException; -import org.opengis.referencing.operation.TransformException; import spark.Request; import java.io.File; @@ -37,7 +35,6 @@ import java.util.stream.Collectors; import java.util.zip.GZIPOutputStream; -import static com.conveyal.file.FileStorageFormat.GEOJSON; import static com.conveyal.file.FileStorageFormat.SHP; import static com.conveyal.r5.analyst.WebMercatorGridPointSet.parseZoom; import static com.conveyal.r5.analyst.progress.WorkProductType.AGGREGATION_AREA; @@ -209,7 +206,7 @@ public void action (ProgressListener progressListener) throws Exception { maskGrid.grid[pixel.x][pixel.y] = pixel.weight * 100_000; }); - AggregationArea aggregationArea = new AggregationArea(userPermissions, name, spatialDataSource); + AggregationArea aggregationArea = new AggregationArea(userPermissions, name, spatialDataSource, zoom); try { File gridFile = FileUtils.createScratchFile("grid"); diff --git a/src/main/java/com/conveyal/analysis/models/AggregationArea.java b/src/main/java/com/conveyal/analysis/models/AggregationArea.java index a0a34be19..d2d62e89b 100644 --- a/src/main/java/com/conveyal/analysis/models/AggregationArea.java +++ b/src/main/java/com/conveyal/analysis/models/AggregationArea.java @@ -22,13 +22,16 @@ public class AggregationArea extends BaseModel { public String dataSourceId; public String dataGroupId; + public int zoom; + /** Zero-argument constructor required for Mongo automatic POJO deserialization. */ public AggregationArea () { } - public AggregationArea(UserPermissions user, String name, SpatialDataSource dataSource) { + public AggregationArea(UserPermissions user, String name, SpatialDataSource dataSource, int zoom) { super(user, name); this.regionId = dataSource.regionId; this.dataSourceId = dataSource._id.toString(); + this.zoom = zoom; } @JsonIgnore From dba9344c15138d5e047b18e7956fa89d0ec7829f Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Thu, 23 Jun 2022 20:55:31 +0800 Subject: [PATCH 04/46] Add an endpoint for deleting tasks (#814) * Add an endpoint for deleting tasks * Increase purge time to 1 day --- .../analysis/components/TaskScheduler.java | 30 ++++++++++++++++++- .../controllers/UserActivityController.java | 22 ++++++++++++-- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/components/TaskScheduler.java b/src/main/java/com/conveyal/analysis/components/TaskScheduler.java index 53ba806d3..9cac27fc1 100644 --- a/src/main/java/com/conveyal/analysis/components/TaskScheduler.java +++ b/src/main/java/com/conveyal/analysis/components/TaskScheduler.java @@ -168,11 +168,39 @@ public final void run () { List apiTaskList = tasks.stream() .map(Task::toApiTask) .collect(Collectors.toUnmodifiableList()); - tasks.removeIf(t -> t.durationComplete().getSeconds() > 60); + // Purge old tasks. + tasks.removeIf(t -> t.durationComplete().toDays() > 3); return apiTaskList; } } + /** + * Return a single task. Does not purge old tasks. + */ + public Task getTaskForUser (String userEmail, String taskId) { + synchronized (tasksForUser) { + Set tasks = tasksForUser.get(userEmail); + if (tasks == null) return null; + for (Task t : tasks) { + if (t.id.toString().equals(taskId)) { + return t; + } + } + return null; + } + } + + /** + * Remove a task. Returns false if task does not exist. Returns true if task exists and is properly removed. + */ + public boolean removeTaskForUser (String userEmail, String taskId) { + synchronized (tasksForUser) { + Set tasks = tasksForUser.get(userEmail); + if (tasks == null) return false; + return tasks.removeIf(t -> t.id.toString().equals(taskId)); + } + } + // Q: Should the caller ever create its own Tasks, or if are tasks created here inside the TaskScheduler from // other raw information? Having the caller creating a Task seems like a good way to configure execution details // like heavy/light/periodic, and submit user information without passing it in. That task request could be separate diff --git a/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java b/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java index ac7b2ecb4..c2beffd30 100644 --- a/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java +++ b/src/main/java/com/conveyal/analysis/controllers/UserActivityController.java @@ -1,15 +1,15 @@ package com.conveyal.analysis.controllers; +import com.conveyal.analysis.AnalysisServerException; import com.conveyal.analysis.UserPermissions; import com.conveyal.analysis.components.TaskScheduler; import com.conveyal.r5.analyst.progress.ApiTask; -import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; +import com.conveyal.r5.analyst.progress.Task; +import com.google.common.collect.ImmutableMap; import spark.Request; import spark.Response; import spark.Service; -import java.util.ArrayList; import java.util.List; import static com.conveyal.analysis.util.JsonUtil.toJson; @@ -40,6 +40,7 @@ public UserActivityController (TaskScheduler taskScheduler) { @Override public void registerEndpoints (Service sparkService) { sparkService.get("/api/activity", this::getActivity, toJson); + sparkService.delete("/api/activity/:id", this::removeActivity, toJson); } private ResponseModel getActivity (Request req, Response res) { @@ -53,6 +54,21 @@ private ResponseModel getActivity (Request req, Response res) { return responseModel; } + private Object removeActivity (Request req, Response res) { + UserPermissions userPermissions = UserPermissions.from(req); + String id = req.params("id"); + Task task = taskScheduler.getTaskForUser(userPermissions.email, id); + // Disallow removing active tasks via the API. + if (task.state.equals(Task.State.ACTIVE)) { + throw AnalysisServerException.badRequest("Cannot clear an active task."); + } + if (taskScheduler.removeTaskForUser(userPermissions.email, id)) { + return ImmutableMap.of("message", "Successfully cleared task."); + } else { + throw AnalysisServerException.badRequest("Failed to clear task."); + } + } + /** API model used only to structure activity JSON messages sent back to UI. */ public static class ResponseModel { /** For example: "Server going down at 17:00 GMT for maintenance" or "Working to resolve known issue [link]." */ From 44d9c1bb637f0944610d168d75c661e8ea493371 Mon Sep 17 00:00:00 2001 From: Anson Stewart Date: Wed, 6 Jul 2022 11:36:19 -0400 Subject: [PATCH 05/46] docs: clarify third-party support and versions in README --- README.md | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index a210c74ec..e139a1122 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ We refer to the routing method as "realistic" because it works by planning door- We say "Real-world and Reimagined" networks because R5's networks are built from widely available open OSM and GTFS data describing baseline transportation systems, but R5 includes a system for applying light-weight patches to those networks for immediate, interactive scenario comparison. -**Please note** that the Conveyal team does not provide technical support for third-party deployments of its analysis platform. We provide paid subscriptions to a cloud-based deployment of this system, which performs these complex calculations hundreds of times faster using a compute cluster. This project is open source primarily to ensure transparency and reproducibility in public planning and decision making processes, and in hopes that it may help researchers, students, and potential collaborators to understand and build upon our methodology. +**Please note** that the Conveyal team does not provide technical support for third-party deployments. R5 is a component of a specialized commercial system, and we align development efforts with our roadmap and the needs of subscribers to our hosted service. This service is designed to facilitate secure online collaboration, user-friendly data management and scenario editing through a web interface, and complex calculations performed hundreds of times faster using a compute cluster. These design goals may not align with other use cases. R5 does not currently expose a stable programming interface ("API" or "SDK"). While the Conveyal team provides ongoing support and compatibility to subscribers, third-party projects using R5 as a library may not work with future releases. As we release new features, previous functions and data types may change. The practical effect is that third-party wrappers or language bindings (e.g. for R or Python) may need to continue using an older release of R5 for feature compatibility (though not necessarily result compatibility, as the methods used in R5 are relatively stable). This project is open source primarily to ensure transparency and reproducibility in public planning and decision making processes, and in hopes that it may help researchers, students, and potential collaborators to understand and build upon our methodology. ## Methodology @@ -19,7 +19,7 @@ For details on the core methods implemented in Conveyal Analysis and R5, see: ### Citations -The Conveyal team is always eager to see cutting-edge uses of our software, so feel free to send us a copy of any thesis, report, or paper produced using this software. We also ask that any academic publications using this software cite the papers above, where relevant and appropriate. +The Conveyal team is always eager to see cutting-edge uses of our software, so feel free to send us a copy of any thesis, report, or paper produced using this software. We also ask that any academic or research publications using this software cite the papers above, where relevant and appropriate. ## Configuration @@ -52,7 +52,7 @@ By default, IntelliJ will follow common Gradle practice and build R5 using the " ## Structured Commit Messages -We use structured commit messages to help generate changelogs and determine version numbers. +We use structured commit messages to help generate changelogs. The first line of these messages is in the following format: `(): ` @@ -68,10 +68,6 @@ The `()` is optional and is often a class name. The `` should be - devops: Changes to code that only affect deployment, logging, etc. No changes to user code. - chore: Any other changes causing no changes to user code. -The body of the commit message (if any) should begin after one blank line. If the commit meets the definition of a major version change according to semantic versioning (e.g. a change in API visible to an external module), the commit message body should begin with `BREAKING CHANGE: `. +The body of the commit message (if any) should begin after one blank line. -Presence of a `fix` commit in a release should increment the number in the third (PATCH) position. -Presence of a `feat` commit in a release should increment the number in the second (MINOR) position. -Presence of a `BREAKING CHANGE` commit in a release should increment the number in the first (MAJOR) position. - -This is based on https://www.conventionalcommits.org. +From 2018 to 2020, we used major/minor/patch release numbering as suggested by https://www.conventionalcommits.org. Starting in 2021, we switched to major/minor release numbering, incrementing the minor version with regular feature releases and the major version only when there are substantial changes to the cluster computing components of our system. Because there is no public API at this time, the conventional definition of breaking changes under semantic versioning does not apply. From 420ec6d035800317ae6540aa2becdf52a5fba10d Mon Sep 17 00:00:00 2001 From: Anson Stewart Date: Tue, 19 Jul 2022 21:59:56 -0400 Subject: [PATCH 06/46] Update README.md --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e139a1122..4085347f7 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,9 @@ We refer to the routing method as "realistic" because it works by planning door- We say "Real-world and Reimagined" networks because R5's networks are built from widely available open OSM and GTFS data describing baseline transportation systems, but R5 includes a system for applying light-weight patches to those networks for immediate, interactive scenario comparison. -**Please note** that the Conveyal team does not provide technical support for third-party deployments. R5 is a component of a specialized commercial system, and we align development efforts with our roadmap and the needs of subscribers to our hosted service. This service is designed to facilitate secure online collaboration, user-friendly data management and scenario editing through a web interface, and complex calculations performed hundreds of times faster using a compute cluster. These design goals may not align with other use cases. R5 does not currently expose a stable programming interface ("API" or "SDK"). While the Conveyal team provides ongoing support and compatibility to subscribers, third-party projects using R5 as a library may not work with future releases. As we release new features, previous functions and data types may change. The practical effect is that third-party wrappers or language bindings (e.g. for R or Python) may need to continue using an older release of R5 for feature compatibility (though not necessarily result compatibility, as the methods used in R5 are relatively stable). This project is open source primarily to ensure transparency and reproducibility in public planning and decision making processes, and in hopes that it may help researchers, students, and potential collaborators to understand and build upon our methodology. +**Please note** that the Conveyal team does not provide technical support for third-party deployments. R5 is a component of a specialized commercial system, and we align development efforts with our roadmap and the needs of subscribers to our hosted service. This service is designed to facilitate secure online collaboration, user-friendly data management and scenario editing through a web interface, and complex calculations performed hundreds of times faster using a compute cluster. These design goals may not align well with other use cases. This project is open source primarily to ensure transparency and reproducibility in public planning and decision making processes, and in hopes that it may help researchers, students, and potential collaborators to understand and build upon our methodology. + +While the Conveyal team provides ongoing support and compatibility to subscribers, third-party projects using R5 as a library may not work with future releases. R5 does not currently expose a stable programming interface ("API" or "SDK"). As we release new features, previous functions and data types may change. The practical effect is that third-party wrappers or language bindings (e.g., for R or Python) may need to continue using an older release of R5 for feature compatibility (though not necessarily result compatibility, as the methods used in R5 are relatively stable). ## Methodology From 25b19f9e98c765e7ed3373b06aee3b0aded9a763 Mon Sep 17 00:00:00 2001 From: Anson Stewart Date: Tue, 26 Jul 2022 13:10:00 -0400 Subject: [PATCH 07/46] Wording change to README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4085347f7..fdd5c00e7 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We say "Real-world and Reimagined" networks because R5's networks are built from **Please note** that the Conveyal team does not provide technical support for third-party deployments. R5 is a component of a specialized commercial system, and we align development efforts with our roadmap and the needs of subscribers to our hosted service. This service is designed to facilitate secure online collaboration, user-friendly data management and scenario editing through a web interface, and complex calculations performed hundreds of times faster using a compute cluster. These design goals may not align well with other use cases. This project is open source primarily to ensure transparency and reproducibility in public planning and decision making processes, and in hopes that it may help researchers, students, and potential collaborators to understand and build upon our methodology. -While the Conveyal team provides ongoing support and compatibility to subscribers, third-party projects using R5 as a library may not work with future releases. R5 does not currently expose a stable programming interface ("API" or "SDK"). As we release new features, previous functions and data types may change. The practical effect is that third-party wrappers or language bindings (e.g., for R or Python) may need to continue using an older release of R5 for feature compatibility (though not necessarily result compatibility, as the methods used in R5 are relatively stable). +While the Conveyal team provides ongoing support and compatibility to subscribers, third-party projects using R5 as a library may not work with future releases. R5 does not currently expose a stable programming interface ("API" or "SDK"). As we release new features, previous functions and data types may change. The practical effect is that third-party wrappers or language bindings (e.g., for R or Python) may need to continue using an older release of R5 for feature compatibility (though not necessarily result compatibility, as the methods used in R5 are now relatively mature). ## Methodology From ace060f0e3bd6929554cd30d0c7edc5205b6ece6 Mon Sep 17 00:00:00 2001 From: ansons Date: Tue, 26 Jul 2022 19:22:07 -0400 Subject: [PATCH 08/46] Increase limit on geographic extents From 250k sq km to 975k sq km. Addresses #815. --- src/main/java/com/conveyal/r5/common/GeometryUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/r5/common/GeometryUtils.java b/src/main/java/com/conveyal/r5/common/GeometryUtils.java index c56c38ea4..c41eef536 100644 --- a/src/main/java/com/conveyal/r5/common/GeometryUtils.java +++ b/src/main/java/com/conveyal/r5/common/GeometryUtils.java @@ -23,8 +23,8 @@ public class GeometryUtils { /** Average of polar and equatorial radii, https://en.wikipedia.org/wiki/Earth */ public static final double RADIUS_OF_EARTH_M = 6_367_450; - /** Maximum area allowed for the bounding box of an uploaded shapefile -- large enough for New York State. */ - private static final double MAX_BOUNDING_BOX_AREA_SQ_KM = 250_000; + /** Maximum area allowed for the bounding box of uploaded files -- large enough for California. */ + private static final double MAX_BOUNDING_BOX_AREA_SQ_KM = 975_000; /** * Haversine formula for distance on the sphere. We used to have a fastDistance function that would estimate this From b980769125b95eefad25d1491be8b75f9a056032 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Wed, 27 Jul 2022 14:20:49 +0800 Subject: [PATCH 09/46] Use the zoom level in aggregation area data group This will help when searching through data groups. Note: since the data group doesn't have any properties itself it must be in the name. --- .../datasource/derivation/AggregationAreaDerivation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java b/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java index dfe994291..dd16c11a1 100644 --- a/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java +++ b/src/main/java/com/conveyal/analysis/datasource/derivation/AggregationAreaDerivation.java @@ -172,7 +172,7 @@ private static void prefetchDataSource (String baseName, String extension, FileS public void action (ProgressListener progressListener) throws Exception { ArrayList aggregationAreas = new ArrayList<>(); - String groupDescription = "Aggregation areas from polygons"; + String groupDescription = "z" + this.zoom + ": aggregation areas"; DataGroup dataGroup = new DataGroup(userPermissions, spatialDataSource._id.toString(), groupDescription); progressListener.beginTask("Reading data source", finalFeatures.size() + 1); From da8f9da09c0050d171497c2a1e917d57bb47ceb6 Mon Sep 17 00:00:00 2001 From: Anson Stewart Date: Wed, 3 Aug 2022 15:16:10 -0400 Subject: [PATCH 10/46] Fix off-by-one in CSV error message The record index starts at the first record, while the source CSV index starts at the header. Also, switching to "near line x" instead of "on line x" for users who may be troubleshooting with spreadsheet software that uses 1-based indexing. --- src/main/java/com/conveyal/r5/analyst/FreeFormPointSet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/r5/analyst/FreeFormPointSet.java b/src/main/java/com/conveyal/r5/analyst/FreeFormPointSet.java index 0cce92dd6..4e9527be5 100644 --- a/src/main/java/com/conveyal/r5/analyst/FreeFormPointSet.java +++ b/src/main/java/com/conveyal/r5/analyst/FreeFormPointSet.java @@ -123,7 +123,7 @@ public static FreeFormPointSet fromCsv ( return ret; } catch (NumberFormatException nfe) { throw new ParameterException( - String.format("Improperly formatted floating point value on line %d of CSV input", rec) + String.format("Improperly formatted floating point value near line %d of CSV input", rec + 1) ); } } From b236f606ee81c787921d2e6dcb9712b7f35f0d1f Mon Sep 17 00:00:00 2001 From: ansons Date: Fri, 2 Sep 2022 16:21:47 -0400 Subject: [PATCH 11/46] Conditionally reduce target number of spot workers at higher zoom levels and for analyses without transit --- .../analysis/components/broker/Broker.java | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index f89f763b0..7bdbdc6b6 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -108,7 +108,8 @@ public interface Config { * Used when auto-starting spot instances. Set to a smaller value to increase the number of * workers requested automatically */ - public final int TARGET_TASKS_PER_WORKER = 800; + public final int TARGET_TASKS_PER_WORKER_TRANSIT = 800; + public final int TARGET_TASKS_PER_WORKER_NONTRANSIT = 4_000; /** * We want to request spot instances to "boost" regional analyses after a few regional task @@ -483,9 +484,27 @@ private void requestExtraWorkersIfAppropriate(Job job) { WorkerCategory workerCategory = job.workerCategory; int categoryWorkersAlreadyRunning = workerCatalog.countWorkersInCategory(workerCategory); if (categoryWorkersAlreadyRunning < MAX_WORKERS_PER_CATEGORY) { - // Start a number of workers that scales with the number of total tasks, up to a fixed number. - // TODO more refined determination of number of workers to start (e.g. using tasks per minute) - int targetWorkerTotal = Math.min(MAX_WORKERS_PER_CATEGORY, job.nTasksTotal / TARGET_TASKS_PER_WORKER); + // TODO more refined determination of number of workers to start (e.g. using observed tasks per minute + // for recently completed tasks -- but what about when initial origins are in a desert/ocean?) + int targetWorkerTotal; + if (job.templateTask.hasTransit()) { + // Total computation for a task with transit depends on the number of stops and whether the + // network has frequency-based routes. The total computation for the job depends on these + // factors as well as the number of tasks (origins). Zoom levels add a complication: the number of + // origins becomes an even poorer proxy for the number of stops. We use a scale factor to compensate + // -- all else equal, high zoom levels imply fewer stops per origin (task) and a lower ideal target + // for number of workers. TODO reduce scale factor further when there are no frequency routes. But is + // this worth adding a field to Job or RegionalTask? + float transitScaleFactor = (9f / job.templateTask.zoom); + targetWorkerTotal = (int) ((job.nTasksTotal / TARGET_TASKS_PER_WORKER_TRANSIT) * transitScaleFactor); + } else { + // Tasks without transit are simpler. They complete relatively quickly, and the total computation for + // the job increases roughly with linearly with the number of origins. + targetWorkerTotal = job.nTasksTotal / TARGET_TASKS_PER_WORKER_NONTRANSIT; + } + + // Do not exceed the limit on workers per category TODO add similar limit per accessGroup or user + targetWorkerTotal = Math.min(targetWorkerTotal, MAX_WORKERS_PER_CATEGORY); // Guardrail until freeform pointsets are tested more thoroughly if (job.templateTask.originPointSet != null) targetWorkerTotal = Math.min(targetWorkerTotal, 5); int nSpot = targetWorkerTotal - categoryWorkersAlreadyRunning; From 5ecbb3e1f2cfe7d8681cc379bbc6e1c462f8ebf5 Mon Sep 17 00:00:00 2001 From: ansons Date: Fri, 2 Sep 2022 16:22:40 -0400 Subject: [PATCH 12/46] Add backoff to avoid hitting EC2 instance limit --- .../com/conveyal/analysis/components/broker/Broker.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index 7bdbdc6b6..14d562e09 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -244,7 +244,8 @@ public void createOnDemandWorkerInCategory(WorkerCategory category, WorkerTags w /** * Create on-demand/spot workers for a given job, after certain checks * @param nOnDemand EC2 on-demand instances to request - * @param nSpot EC2 spot instances to request + * @param nSpot Target number of EC2 spot instances to request. The actual number requested may be lower if the + * total number of workers running is approaching the maximum specified in the Broker config. */ public void createWorkersInCategory (WorkerCategory category, WorkerTags workerTags, int nOnDemand, int nSpot) { @@ -258,6 +259,12 @@ public void createWorkersInCategory (WorkerCategory category, WorkerTags workerT return; } + // Backoff: reduce the nSpot requested when the number of already running workers starts approaching the + // configured maximum + if (workerCatalog.totalWorkerCount() * 2 > config.maxWorkers()) { + nSpot = Math.min(nSpot, (config.maxWorkers() - workerCatalog.totalWorkerCount()) / 2); + } + if (workerCatalog.totalWorkerCount() + nOnDemand + nSpot >= config.maxWorkers()) { String message = String.format( "Maximum of %d workers already started, not starting more;" + From 9b544458177e3f6de68de06f70f010a635d2ec41 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Tue, 20 Sep 2022 20:02:06 +0800 Subject: [PATCH 13/46] handle missing restriction vertex, fixes #681 --- .../java/com/conveyal/r5/streets/StreetLayer.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/r5/streets/StreetLayer.java b/src/main/java/com/conveyal/r5/streets/StreetLayer.java index 370d26452..87e176c64 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetLayer.java +++ b/src/main/java/com/conveyal/r5/streets/StreetLayer.java @@ -871,7 +871,12 @@ else if ("via".equals(member.role)) { final long fromWayId = from.id; // more effectively final nonsense final boolean[] bad = new boolean[] { false }; - int fromVertex = vertexIndexForOsmNode.get(pathNodes[0]); + final long fromNode = pathNodes[0]; + final int fromVertex = vertexIndexForOsmNode.get(fromNode); + if (fromVertex == -1) { + LOG.warn("Vertex not found for from-node {} of restriction {}, skipping this restriction", fromNode, osmRelationId); + return; + } // find the edges incomingEdges.get(fromVertex).forEach(eidx -> { @@ -889,7 +894,12 @@ else if ("via".equals(member.role)) { return true; // iteration should continue }); - int toVertex = vertexIndexForOsmNode.get(pathNodes[pathNodes.length - 1]); + final long toNode = pathNodes[pathNodes.length - 1]; + final int toVertex = vertexIndexForOsmNode.get(toNode); + if (toVertex == -1) { + LOG.warn("Vertex not found for to-node {} of restriction {}, skipping this restriction", toNode, osmRelationId); + return; + } final int[] toEdge = new int[] { -1 }; final long toWayId = to.id; // more effectively final nonsense From d41ae5b6a05ba9d88a91908195304ad571547a22 Mon Sep 17 00:00:00 2001 From: Anson Stewart Date: Wed, 21 Sep 2022 00:12:22 -0400 Subject: [PATCH 14/46] Log when spot instance request is reduced Co-authored-by: Andrew Byrd --- .../java/com/conveyal/analysis/components/broker/Broker.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index 14d562e09..ee1193b3d 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -263,6 +263,7 @@ public void createWorkersInCategory (WorkerCategory category, WorkerTags workerT // configured maximum if (workerCatalog.totalWorkerCount() * 2 > config.maxWorkers()) { nSpot = Math.min(nSpot, (config.maxWorkers() - workerCatalog.totalWorkerCount()) / 2); + LOG.info("Worker pool over half of maximum size. Number of new spot instances set to {}", nSpot); } if (workerCatalog.totalWorkerCount() + nOnDemand + nSpot >= config.maxWorkers()) { From ea9b5a36238c3db0c4fe0de7f3fde92890f6bc53 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Thu, 27 Oct 2022 12:47:02 +0800 Subject: [PATCH 15/46] Path summaries: return raw data in JSON (#827) * Paths: return machine usable results Returns times in seconds instead of converting before sending. * Improve stopString and routeString outputs If we are returning a single string, concatenating values can be very redundant as IDs may be the same as `short_name`s and stop indexes would need to be parsed to be useful. * Round the minutes in the SimpsonDesertTests Should the tests be using the "HumanReadableIteration" results? --- .../r5/analyst/cluster/AnalysisWorker.java | 9 +- .../r5/analyst/cluster/PathResult.java | 42 +---- .../r5/analyst/cluster/PathResultSummary.java | 177 ++++++++++++++++++ .../com/conveyal/r5/transit/RouteInfo.java | 12 +- .../com/conveyal/r5/transit/TransitLayer.java | 2 - .../r5/transit/path/RouteSequence.java | 6 +- .../analyst/network/SimpsonDesertTests.java | 13 +- 7 files changed, 214 insertions(+), 47 deletions(-) create mode 100644 src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java b/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java index f34ee09ee..aad7da99d 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java @@ -38,7 +38,6 @@ import java.time.format.DateTimeFormatter; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.Random; import java.util.UUID; @@ -348,7 +347,7 @@ private byte[] singlePointResultToBinary ( oneOriginResult.accessibility, transportNetwork.scenarioApplicationWarnings, transportNetwork.scenarioApplicationInfo, - oneOriginResult.paths + oneOriginResult.paths != null ? new PathResultSummary(oneOriginResult.paths, transportNetwork.transitLayer) : null ); } // Single-point tasks don't have a job ID. For now, we'll categorize them by scenario ID. @@ -488,7 +487,7 @@ public static class GridJsonBlock { public List scenarioApplicationInfo; - public List pathSummaries; + public PathResultSummary pathSummaries; @Override public String toString () { @@ -515,7 +514,7 @@ public static void addJsonToGrid ( AccessibilityResult accessibilityResult, List scenarioApplicationWarnings, List scenarioApplicationInfo, - PathResult pathResult + PathResultSummary pathResult ) throws IOException { var jsonBlock = new GridJsonBlock(); jsonBlock.scenarioApplicationInfo = scenarioApplicationInfo; @@ -526,7 +525,7 @@ public static void addJsonToGrid ( // study area). But we'd need to control the number of decimal places serialized into the JSON. jsonBlock.accessibility = accessibilityResult.getIntValues(); } - jsonBlock.pathSummaries = pathResult == null ? Collections.EMPTY_LIST : pathResult.getPathIterationsForDestination(); + jsonBlock.pathSummaries = pathResult; LOG.debug("Travel time surface written, appending {}.", jsonBlock); // We could do this when setting up the Spark handler, supplying writeValue as the response transformer // But then you also have to handle the case where you are returning raw bytes. diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java b/src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java index dd268ce95..7d523c4e4 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java @@ -1,5 +1,6 @@ package com.conveyal.r5.analyst.cluster; +import com.conveyal.r5.analyst.StreetTimesAndModes; import com.conveyal.r5.transit.TransitLayer; import com.conveyal.r5.transit.path.Path; import com.conveyal.r5.transit.path.PatternSequence; @@ -11,7 +12,6 @@ import org.apache.commons.lang3.ArrayUtils; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.List; @@ -46,7 +46,7 @@ public class PathResult { * details. For now, the path template is a route-based path ignoring per-iteration details such as wait time. * With additional changes, patterns could be collapsed further to route combinations or modes. */ - private final Multimap[] iterationsForPathTemplates; + public final Multimap[] iterationsForPathTemplates; private final TransitLayer transitLayer; public static String[] DATA_COLUMNS = new String[]{ @@ -153,21 +153,16 @@ public enum Stat { * Wraps path and iteration details for JSON serialization */ public static class PathIterations { - public String access; // StreetTimesAndModes.StreetTimeAndMode would be more machine-readable. - public String egress; + public StreetTimesAndModes.StreetTimeAndMode access; + public StreetTimesAndModes.StreetTimeAndMode egress; public Collection transitLegs; - public Collection iterations; + public Collection iterations; PathIterations(RouteSequence pathTemplate, TransitLayer transitLayer, Collection iterations) { - this.access = pathTemplate.stopSequence.access == null ? null : pathTemplate.stopSequence.access.toString(); - this.egress = pathTemplate.stopSequence.egress == null ? null : pathTemplate.stopSequence.egress.toString(); + this.access = pathTemplate.stopSequence.access; + this.egress = pathTemplate.stopSequence.egress; this.transitLegs = pathTemplate.transitLegs(transitLayer); - this.iterations = iterations.stream().map(HumanReadableIteration::new).collect(Collectors.toList()); - iterations.forEach(pathTemplate.stopSequence::transferTime); // The transferTime method includes an - // assertion that the transfer time is non-negative, i.e. that the access + egress + wait + ride times of - // a specific iteration do not exceed the total travel time. Perform that sense check here, even though - // the transfer time is not reported to the front-end for the human-readable single-point responses. - // TODO add transferTime to HumanReadableIteration? + this.iterations = iterations; } } @@ -213,25 +208,4 @@ public Iteration(int totalTime) { this.totalTime = totalTime; } } - - /** - * Timestamp style clock times, and rounded wait/total time, for inspection as JSON. - */ - public static class HumanReadableIteration { - public String departureTime; - public double[] waitTimes; - public double totalTime; - - HumanReadableIteration(Iteration iteration) { - // TODO track departure time for non-transit paths (so direct trips don't show departure time 00:00). - this.departureTime = - String.format("%02d:%02d", Math.floorDiv(iteration.departureTime, 3600), - (int) (iteration.departureTime / 60.0 % 60)); - this.waitTimes = Arrays.stream(iteration.waitTimes.toArray()).mapToDouble( - wait -> Math.round(wait / 60f * 10) / 10.0 - ).toArray(); - this.totalTime = Math.round(iteration.totalTime / 60f * 10) / 10.0; - } - } - } diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java b/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java new file mode 100644 index 000000000..812592eb2 --- /dev/null +++ b/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java @@ -0,0 +1,177 @@ +package com.conveyal.r5.analyst.cluster; + +import com.conveyal.r5.analyst.StreetTimesAndModes; +import com.conveyal.r5.transit.TransitLayer; +import com.conveyal.r5.transit.path.StopSequence; +import gnu.trove.list.TIntList; +import gnu.trove.list.array.TIntArrayList; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * Convert a PathResult to an API friendly version. Uses the transit layer to get route and stop details. + */ +public class PathResultSummary { + public List iterations = new ArrayList<>(); + public List itineraries = new ArrayList<>(); + public int fastestPathSeconds = Integer.MAX_VALUE; + + public PathResultSummary( + PathResult pathResult, + TransitLayer transitLayer + ) { + if (pathResult == null || pathResult.iterationsForPathTemplates.length != 1 || pathResult.iterationsForPathTemplates[0] == null) + return; + + // Iterate through each path result creating a list of iteration details and itineraries that reference each + // other through an index. + int itineraryIndex = 0; + for (var pathTemplate : pathResult.iterationsForPathTemplates[0].keySet()) { + var allIterations = pathResult.iterationsForPathTemplates[0].get(pathTemplate); + TIntArrayList durations = new TIntArrayList(); + int fastestIteration = Integer.MAX_VALUE; + for (var iteration : allIterations) { + if (iteration.totalTime < fastestIteration) { + fastestIteration = iteration.totalTime; + if (fastestIteration < fastestPathSeconds) fastestPathSeconds = fastestIteration; + } + // Add to the set of durations + durations.add(iteration.totalTime); + IterationDetails iterationDetails = new IterationDetails( + iteration.departureTime, + iteration.waitTimes, + iteration.totalTime, + itineraryIndex + ); + // Add to the list of departure times + this.iterations.add(iterationDetails); + } + List transitLegs = new ArrayList<>(); + for (int routeIndex = 0; routeIndex < pathTemplate.routes.size(); routeIndex++) { + transitLegs.add(new TransitLeg(transitLayer, pathTemplate.stopSequence, routeIndex)); + } + itineraries.add(new Itinerary( + pathTemplate.stopSequence.access, + pathTemplate.stopSequence.egress, + transitLegs, + allIterations.size(), + durations.min(), + durations.max(), + itineraryIndex + )); + itineraryIndex += 1; + } + // Sort the iterations (by departure time and itinerary index) + Collections.sort(iterations); + } + + /** + * An itinerary taken from a path result, including access and egress mode, transit legs, duration range (seconds), + * and how many iterations this itinerary was used. + */ + static class Itinerary { + public StreetTimesAndModes.StreetTimeAndMode access; + public StreetTimesAndModes.StreetTimeAndMode egress; + public List transitLegs; + public int iterationsOptimal; + public int minSeconds; + public int maxSeconds; + + public int index; + + Itinerary( + StreetTimesAndModes.StreetTimeAndMode access, + StreetTimesAndModes.StreetTimeAndMode egress, + List transitLegs, + int iterationsOptimal, + int minSeconds, + int maxSeconds, + int index + ) { + this.access = access; + this.egress = egress; + this.transitLegs = transitLegs; + this.iterationsOptimal = iterationsOptimal; + this.maxSeconds = maxSeconds; + this.minSeconds = minSeconds; + this.index = index; + } + } + + /** + * String representations of the boarding stop, alighting stop, and route, with ride time (in seconds). + */ + static class TransitLeg { + public String routeId; + public String routeName; + public int rideTimeSeconds; + public String boardStopId; + public String boardStopName; + + public String alightStopId; + public String alightStopName; + + public TransitLeg( + TransitLayer transitLayer, + StopSequence stopSequence, + int routeIndex + ) { + var routeInfo = transitLayer.routes.get(routeIndex); + routeId = routeInfo.route_id; + routeName = routeInfo.getName(); + + rideTimeSeconds = stopSequence.rideTimesSeconds.get(routeIndex); + + int boardStopIndex = stopSequence.boardStops.get(routeIndex); + boardStopId = getStopId(transitLayer, boardStopIndex); + boardStopName = transitLayer.stopNames.get(boardStopIndex); + + int alightStopIndex = stopSequence.alightStops.get(routeIndex); + alightStopId = getStopId(transitLayer, alightStopIndex); + alightStopName = transitLayer.stopNames.get(alightStopIndex); + } + } + + /** + * Temporal details of a specific iteration of our RAPTOR implementation (per-leg wait times and total time + * implied by a specific departure time and randomized schedule offsets). + *

+ * All times are in seconds. Departure time is seconds from midnight. + */ + public static class IterationDetails implements Comparable { + public final int departureTime; + public final int[] waitTimes; + public final int totalWaitTime; + public final int totalTime; + public final int itineraryIndex; + + public IterationDetails(int departureTime, TIntList waitTimes, int totalTime, int itineraryIndex) { + this.departureTime = departureTime; + this.waitTimes = waitTimes.toArray(); + this.totalTime = totalTime; + this.totalWaitTime = waitTimes.sum(); + this.itineraryIndex = itineraryIndex; + } + + @Override + public int compareTo(IterationDetails o) { + int diff = this.departureTime - o.departureTime; + if (diff != 0) return diff; + return this.itineraryIndex - o.itineraryIndex; + } + } + + /** + * Get the stop ID. If the ID does not exist, the stop is a new one added by a modification, so return "new". + * + * @param stopIndex + * @return stopId + */ + private static String getStopId(TransitLayer transitLayer, int stopIndex) { + String stopId = transitLayer.stopIdForIndex.get(stopIndex); + if (stopId == null) return "[new]"; + return stopId; + } +} \ No newline at end of file diff --git a/src/main/java/com/conveyal/r5/transit/RouteInfo.java b/src/main/java/com/conveyal/r5/transit/RouteInfo.java index 3617fa355..484559a1d 100644 --- a/src/main/java/com/conveyal/r5/transit/RouteInfo.java +++ b/src/main/java/com/conveyal/r5/transit/RouteInfo.java @@ -33,5 +33,15 @@ public RouteInfo (Route route, Agency agency) { this.agency_url = route.route_url; } - public RouteInfo () { /* do nothing */ } + public RouteInfo() { /* do nothing */ } + + public String getName() { + if (route_short_name != null) { + return route_short_name; + } else if (route_long_name != null) { + return route_long_name; + } else { + return route_id; + } + } } diff --git a/src/main/java/com/conveyal/r5/transit/TransitLayer.java b/src/main/java/com/conveyal/r5/transit/TransitLayer.java index 2b174a8b5..e140b0d89 100644 --- a/src/main/java/com/conveyal/r5/transit/TransitLayer.java +++ b/src/main/java/com/conveyal/r5/transit/TransitLayer.java @@ -46,7 +46,6 @@ import java.util.BitSet; import java.util.Collection; import java.util.Collections; -import java.util.EnumSet; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -841,5 +840,4 @@ public String stopString(int stopIndex, boolean includeName) { if (includeName) stop += " (" + stopNames.get(stopIndex) + ")"; return stop; } - } diff --git a/src/main/java/com/conveyal/r5/transit/path/RouteSequence.java b/src/main/java/com/conveyal/r5/transit/path/RouteSequence.java index 6e3e07037..6ed2eb73c 100644 --- a/src/main/java/com/conveyal/r5/transit/path/RouteSequence.java +++ b/src/main/java/com/conveyal/r5/transit/path/RouteSequence.java @@ -12,8 +12,10 @@ /** A door-to-door path that includes the routes ridden between stops */ public class RouteSequence { - /** Route indexes (those used in R5 transit layer) for each transit leg */ - private final TIntList routes; + /** + * Route indexes (those used in R5 transit layer) for each transit leg + */ + public final TIntList routes; public final StopSequence stopSequence; /** Convert a pattern-based path into a more general route-based path. */ 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 327716429..f1ff7ed96 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java +++ b/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java @@ -146,6 +146,13 @@ public void testGridFrequencyAlternatives () throws Exception { DistributionTester.assertExpectedDistribution(twoAlternatives, travelTimePercentiles); } + /** + * For evaluating results from the tests below. + */ + private int[] roundPathTimesToMinutes (PathResult.PathIterations paths) { + return paths.iterations.stream().mapToInt(i -> (int) (Math.round(i.totalTime / 60f * 10) / 10.0)).toArray(); + } + /** * Test that the router correctly handles overtaking trips on the same route. Consider Trip A and Trip B, on a * horizontal route where each hop time is 30 seconds, except when Trip A slows to 10 minutes/hop for hops 20 and @@ -187,7 +194,7 @@ public void testOvertakingCases () throws Exception { OneOriginResult standardResult = new TravelTimeComputer(standardRider, network).computeTravelTimes(); List standardPaths = standardResult.paths.getPathIterationsForDestination(); - int[] standardTimes = standardPaths.get(0).iterations.stream().mapToInt(i -> (int) i.totalTime).toArray(); + int[] standardTimes = roundPathTimesToMinutes(standardPaths.get(0)); // Trip B departs stop 30 at 7:35. So 30-35 minute wait, plus ~5 minute ride and ~5 minute egress leg assertTrue(Arrays.equals(new int[]{45, 44, 43, 42, 41}, standardTimes)); @@ -198,7 +205,7 @@ public void testOvertakingCases () throws Exception { OneOriginResult naiveResult = new TravelTimeComputer(naiveRider, network).computeTravelTimes(); List naivePaths = naiveResult.paths.getPathIterationsForDestination(); - int[] naiveTimes = naivePaths.get(0).iterations.stream().mapToInt(i -> (int) i.totalTime).toArray(); + int[] naiveTimes = roundPathTimesToMinutes(naivePaths.get(0)); // Trip A departs stop 10 at 7:15. So 10-15 minute wait, plus ~35 minute ride and ~5 minute egress leg assertTrue(Arrays.equals(new int[]{54, 53, 52, 51, 50}, naiveTimes)); @@ -210,7 +217,7 @@ public void testOvertakingCases () throws Exception { OneOriginResult savvyResult = new TravelTimeComputer(savvyRider, network).computeTravelTimes(); List savvyPaths = savvyResult.paths.getPathIterationsForDestination(); - int[] savvyTimes = savvyPaths.get(0).iterations.stream().mapToInt(i -> (int) i.totalTime).toArray(); + int[] savvyTimes = roundPathTimesToMinutes(savvyPaths.get(0)); // Trip B departs stop 10 at 7:25. So 8-12 minute wait, plus ~16 minute ride and ~5 minute egress leg assertTrue(Arrays.equals(new int[]{32, 31, 30, 29, 28}, savvyTimes)); } From e7fa66c7f5c2c54f4030f76916515f93c8c50718 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 27 Oct 2022 20:37:58 +0800 Subject: [PATCH 16/46] remove vestiges of human-readable display --- .../r5/analyst/network/SimpsonDesertTests.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) 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 f1ff7ed96..c6e612778 100644 --- a/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java +++ b/src/test/java/com/conveyal/r5/analyst/network/SimpsonDesertTests.java @@ -17,6 +17,7 @@ import java.util.List; import java.util.Map; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -149,8 +150,8 @@ public void testGridFrequencyAlternatives () throws Exception { /** * For evaluating results from the tests below. */ - private int[] roundPathTimesToMinutes (PathResult.PathIterations paths) { - return paths.iterations.stream().mapToInt(i -> (int) (Math.round(i.totalTime / 60f * 10) / 10.0)).toArray(); + private static double[] pathTimesAsMinutes (PathResult.PathIterations paths) { + return paths.iterations.stream().mapToDouble(i -> i.totalTime / 60d).toArray(); } /** @@ -194,9 +195,8 @@ public void testOvertakingCases () throws Exception { OneOriginResult standardResult = new TravelTimeComputer(standardRider, network).computeTravelTimes(); List standardPaths = standardResult.paths.getPathIterationsForDestination(); - int[] standardTimes = roundPathTimesToMinutes(standardPaths.get(0)); // Trip B departs stop 30 at 7:35. So 30-35 minute wait, plus ~5 minute ride and ~5 minute egress leg - assertTrue(Arrays.equals(new int[]{45, 44, 43, 42, 41}, standardTimes)); + assertArrayEquals(new double[]{45.0, 44.0, 43.0, 42.0, 41.0}, pathTimesAsMinutes(standardPaths.get(0)), 0.3); // 2. Naive rider: downstream overtaking means Trip A departs origin first but is not fastest to destination. AnalysisWorkerTask naiveRider = gridLayout.copyTask(standardRider) @@ -205,9 +205,8 @@ public void testOvertakingCases () throws Exception { OneOriginResult naiveResult = new TravelTimeComputer(naiveRider, network).computeTravelTimes(); List naivePaths = naiveResult.paths.getPathIterationsForDestination(); - int[] naiveTimes = roundPathTimesToMinutes(naivePaths.get(0)); // Trip A departs stop 10 at 7:15. So 10-15 minute wait, plus ~35 minute ride and ~5 minute egress leg - assertTrue(Arrays.equals(new int[]{54, 53, 52, 51, 50}, naiveTimes)); + assertArrayEquals(new double[]{54.0, 53.0, 52.0, 51.0, 50.0}, pathTimesAsMinutes(naivePaths.get(0)), 0.3); // 3. Savvy rider (look-ahead abilities from starting the trip 13 minutes later): waits to board Trip B, even // when boarding Trip A is possible @@ -217,9 +216,8 @@ public void testOvertakingCases () throws Exception { OneOriginResult savvyResult = new TravelTimeComputer(savvyRider, network).computeTravelTimes(); List savvyPaths = savvyResult.paths.getPathIterationsForDestination(); - int[] savvyTimes = roundPathTimesToMinutes(savvyPaths.get(0)); // Trip B departs stop 10 at 7:25. So 8-12 minute wait, plus ~16 minute ride and ~5 minute egress leg - assertTrue(Arrays.equals(new int[]{32, 31, 30, 29, 28}, savvyTimes)); + assertArrayEquals(new double[]{32.0, 31.0, 30.0, 29.0, 28.0}, pathTimesAsMinutes(savvyPaths.get(0)), 0.3); } /** From 210b4c9d769f383de2a3d3c7e89b0f38993e3725 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 27 Oct 2022 20:51:27 +0800 Subject: [PATCH 17/46] remove jitpack maven repo I did a local build after wiping out my gradle caches directory and it didn't seem to fetch anything from jitpack that's not also on the maven central repo. --- build.gradle | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/build.gradle b/build.gradle index 107398f22..fee0a0898 100644 --- a/build.gradle +++ b/build.gradle @@ -123,10 +123,8 @@ repositories { // Put Open Source Geospatial before Maven Central to get JAI core, see https://stackoverflow.com/a/26993223 maven { url 'https://repo.osgeo.org/repository/release/' } mavenCentral() - // TODO review whether we really need these repositories + // TODO review whether we really need the repositories below maven { url 'https://maven.conveyal.com' } - // Used for importing java projects from github (why do we need this?) - maven { url 'https://jitpack.io' } // For the polyline encoder maven { url 'https://nexus.axiomalaska.com/nexus/content/repositories/public-releases' } } From e87e4fd669c7e3ae7e0b5787974be2324c6b2bf7 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Thu, 27 Oct 2022 23:13:36 +0800 Subject: [PATCH 18/46] Use the status code from the worker response (#829) Fixes #828 --- .../conveyal/analysis/controllers/WorkerProxyController.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/controllers/WorkerProxyController.java b/src/main/java/com/conveyal/analysis/controllers/WorkerProxyController.java index dc456d8b5..d470427da 100644 --- a/src/main/java/com/conveyal/analysis/controllers/WorkerProxyController.java +++ b/src/main/java/com/conveyal/analysis/controllers/WorkerProxyController.java @@ -23,8 +23,6 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; -import static com.conveyal.analysis.util.HttpStatus.OK_200; - /** * This proxies requests coming from the UI over to any currently active worker for the specified network bundle. * This could be used for point-to-point routing or the existing R5 endpoints producing debug tiles of the graph. @@ -104,7 +102,7 @@ private Object proxyRequest (Request request, Response response) throws IOExcept resp.headers().map().forEach((key, value) -> { if (!value.isEmpty()) response.header(key, value.get(0)); }); - response.status(OK_200); + response.status(resp.statusCode()); return resp.body(); } catch (Exception exception) { response.status(HttpStatus.BAD_REQUEST_400); From 44c683941de3035bb4f5de0d60e71bbba27db29f Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Fri, 28 Oct 2022 18:58:49 +0800 Subject: [PATCH 19/46] Add CodeQL Analysis Github Action (#809) * Create codeql-analysis.yml * CodeQL Fix: explicitly cast long to int * CodeQL Fix: prevent "zip slip" * Add comment about CodeQL action --- .github/workflows/codeql-analysis.yml | 73 +++++++++++++++++++ .../com/conveyal/r5/streets/StreetRouter.java | 5 +- .../r5/transit/TransportNetworkCache.java | 7 +- 3 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/codeql-analysis.yml diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml new file mode 100644 index 000000000..b3db3cf8c --- /dev/null +++ b/.github/workflows/codeql-analysis.yml @@ -0,0 +1,73 @@ +# This file was created by CodeQL for this repository. The only change was +# removing 'python' and 'javascript' from the language array. +# +# For most projects, this workflow file will not need changing; you simply need +# to commit it to your repository. +# +# You may wish to alter this file to override the set of languages analyzed, +# or to provide custom queries or build logic. +# +# ******** NOTE ******** +# We have attempted to detect the languages in your repository. Please check +# the `language` matrix defined below to confirm you have the correct set of +# supported CodeQL languages. +# +name: "CodeQL" + +on: + push: + branches: [ dev ] + pull_request: + # The branches below must be a subset of the branches above + branches: [ dev ] + schedule: + - cron: '24 12 * * 1' + +jobs: + analyze: + name: Analyze + runs-on: ubuntu-latest + permissions: + actions: read + contents: read + security-events: write + + strategy: + fail-fast: false + matrix: + language: [ 'java' ] + # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python', 'ruby' ] + # Learn more about CodeQL language support at https://aka.ms/codeql-docs/language-support + + steps: + - name: Checkout repository + uses: actions/checkout@v3 + + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v2 + with: + languages: ${{ matrix.language }} + # If you wish to specify custom queries, you can do so here or in a config file. + # By default, queries listed here will override any specified in a config file. + # Prefix the list here with "+" to use these queries and those in the config file. + # queries: ./path/to/local/query, your-org/your-repo/queries@main + + # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). + # If this step fails, then you should remove it and run the build manually (see below) + - name: Autobuild + uses: github/codeql-action/autobuild@v2 + + # ℹ️ Command-line programs to run using the OS shell. + # 📚 See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun + + # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines + # and modify them (or add more) to build your code if your project + # uses a compiled language + + #- run: | + # make bootstrap + # make release + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v2 diff --git a/src/main/java/com/conveyal/r5/streets/StreetRouter.java b/src/main/java/com/conveyal/r5/streets/StreetRouter.java index 71038f4d9..fa3750e35 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetRouter.java +++ b/src/main/java/com/conveyal/r5/streets/StreetRouter.java @@ -35,7 +35,6 @@ import java.util.List; import java.util.PriorityQueue; -import static com.conveyal.r5.common.Util.isNullOrEmpty; import static com.conveyal.r5.common.Util.notNullOrEmpty; import static com.conveyal.r5.streets.LinkedPointSet.OFF_STREET_SPEED_MILLIMETERS_PER_SECOND; import static gnu.trove.impl.Constants.DEFAULT_CAPACITY; @@ -968,8 +967,8 @@ public void incrementTimeInSeconds(long seconds) { durationSeconds-=seconds; durationFromOriginSeconds -= seconds; } else { - durationSeconds+=seconds; - durationFromOriginSeconds += seconds; + durationSeconds += (int) seconds; + durationFromOriginSeconds += (int) seconds; } } diff --git a/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java b/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java index 4ee058066..d93891ba1 100644 --- a/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java +++ b/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java @@ -6,8 +6,8 @@ import com.conveyal.file.FileStorageKey; import com.conveyal.file.FileUtils; import com.conveyal.gtfs.GTFSCache; -import com.conveyal.r5.analyst.cluster.TransportNetworkConfig; import com.conveyal.r5.analyst.cluster.ScenarioCache; +import com.conveyal.r5.analyst.cluster.TransportNetworkConfig; import com.conveyal.r5.analyst.scenario.Modification; import com.conveyal.r5.analyst.scenario.RasterCost; import com.conveyal.r5.analyst.scenario.Scenario; @@ -15,7 +15,6 @@ import com.conveyal.r5.common.JsonUtilities; import com.conveyal.r5.kryo.KryoNetworkSerializer; import com.conveyal.r5.profile.StreetMode; -import com.conveyal.r5.shapefile.ShapefileMatcher; import com.conveyal.r5.streets.OSMCache; import com.conveyal.r5.streets.StreetLayer; import com.github.benmanes.caffeine.cache.Caffeine; @@ -207,6 +206,10 @@ private TransportNetwork buildNetworkFromBundleZip (String networkId) { ZipEntry entry; while ((entry = zis.getNextEntry()) != null) { File entryDestination = new File(dataDirectory, entry.getName()); + if (!entryDestination.toPath().normalize().startsWith(dataDirectory.toPath())) { + throw new Exception("Bad zip entry"); + } + // Are both these mkdirs calls necessary? entryDestination.getParentFile().mkdirs(); if (entry.isDirectory()) From 76642abe643579aaec263f4c054dc2da83d16b7e Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 21 Dec 2022 02:28:28 +0800 Subject: [PATCH 20/46] Add table of per-edge factors to network when modification requires it (#838) * add per-edge cost table to network when needed this is currently modifying the scenario network in the resolve phase which is probably bad form - ideally we'd shift this to the apply phase. * revise EdgeTraversalTimes creation in ModifyStreets switch to factory method for EdgeTraversalTimes Create EdgeTraversalTimes in apply phase instead of resolve phase change info message grammar to work with "Modification X" as a header * Add simple test for a Modify Streets modification Goal was to create a test would have failed before this PR. It tests general effect of applying a modification with a walk cost factor by checking if there were decreased time to reach routed nodes vs a baseline network. * fix iteration over found vertices use inequalities for softer assertions style changes, use Arrays.asList() Co-authored-by: Trevor Gerhardt --- .../r5/analyst/scenario/AdjustSpeed.java | 2 +- .../r5/analyst/scenario/ModifyStreets.java | 33 +++++- .../com/conveyal/r5/streets/EdgeStore.java | 4 +- .../r5/streets/EdgeTraversalTimes.java | 22 +++- .../r5/streets/SingleModeTraversalTimes.java | 7 +- .../analyst/scenario/ModifyStreetsTest.java | 112 ++++++++++++++++++ 6 files changed, 166 insertions(+), 14 deletions(-) create mode 100644 src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java index 6a6f9b825..6121c7e63 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java +++ b/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."); } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java b/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java index c8c6f5f09..5dc51bf7f 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java +++ b/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; /** + *

* This modification selects all edges inside a given set of polygons and changes their characteristics. + *

+ * 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). + *

+ * 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: + *

    + *
  • 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.
    • + *
  • 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.
    • + *
*/ 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(); ) { diff --git a/src/main/java/com/conveyal/r5/streets/EdgeStore.java b/src/main/java/com/conveyal/r5/streets/EdgeStore.java index 9aec7a5d0..9ed498537 100644 --- a/src/main/java/com/conveyal/r5/streets/EdgeStore.java +++ b/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); diff --git a/src/main/java/com/conveyal/r5/streets/EdgeTraversalTimes.java b/src/main/java/com/conveyal/r5/streets/EdgeTraversalTimes.java index 1195be2d4..07d515bdf 100644 --- a/src/main/java/com/conveyal/r5/streets/EdgeTraversalTimes.java +++ b/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; + } + } diff --git a/src/main/java/com/conveyal/r5/streets/SingleModeTraversalTimes.java b/src/main/java/com/conveyal/r5/streets/SingleModeTraversalTimes.java index c4bee3042..fb8e25052 100644 --- a/src/main/java/com/conveyal/r5/streets/SingleModeTraversalTimes.java +++ b/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); diff --git a/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java b/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java new file mode 100644 index 000000000..9da5336b5 --- /dev/null +++ b/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(); + } +} \ No newline at end of file From bed8ca869a1644f164097fd550a8d3a26ec8d35a Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 21 Dec 2022 14:09:31 +0800 Subject: [PATCH 21/46] avoid multiple LTS flags on edge in modifications (#843) add method that clears all LTS flags before setting a new one use this method in ModifyStreets and ShapefileMatcher add and clarify code comments refactoring: add static imports of enum values for readability add braces around conditional blocks the LevelOfTrafficStressLabeler may still set multiple flags during initial network build, checks and normalization could be added. --- .../r5/analyst/scenario/ModifyStreets.java | 5 +- .../r5/analyst/scenario/ShapefileLts.java | 4 +- .../labeling/LevelOfTrafficStressLabeler.java | 146 ++++++++-------- .../r5/shapefile/ShapefileMatcher.java | 15 +- .../com/conveyal/r5/streets/EdgeStore.java | 32 ++++ .../analyst/scenario/ModifyStreetsTest.java | 158 +++++++++++++----- src/test/resources/manual/README.txt | 3 + 7 files changed, 229 insertions(+), 134 deletions(-) create mode 100644 src/test/resources/manual/README.txt diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java b/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java index 5dc51bf7f..e436d2e8f 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java @@ -21,7 +21,7 @@ import java.util.EnumSet; import java.util.List; -import static com.conveyal.r5.labeling.LevelOfTrafficStressLabeler.intToLts; +import static com.conveyal.r5.streets.EdgeStore.intToLts; /** *

@@ -234,8 +234,7 @@ private void handleOneEdge (EdgeStore.Edge newEdge, EdgeStore.Edge oldEdge) { newEdge.disallowAllModes(); newEdge.allowStreetModes(allowedModes); if (bikeLts != null) { - // Overwrite the LTS copied in the flags - newEdge.setFlag(intToLts(bikeLts)); + newEdge.setLts(bikeLts); } if (carSpeedKph != null) { newEdge.setSpeedKph(carSpeedKph); diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java b/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java index 65d20b33d..e39163e29 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java @@ -55,7 +55,9 @@ public boolean resolve (TransportNetwork network) { @Override public boolean apply (TransportNetwork network) { // Replicate the entire flags array so we can write to it (following copy-on-write policy). - // Otherwise the TIntAugmentedList only allows extending the base graph. + // Otherwise the TIntAugmentedList only allows extending the base graph. An alternative approach can be seen in + // ModifyStreets, where all affected edges are marked deleted and then recreated in the augmented lists. + // The appraoch here assumes a high percentage of edges changed, while ModifyStreets assumes a small percentage. network.streetLayer.edgeStore.flags = new TIntArrayList(network.streetLayer.edgeStore.flags); ShapefileMatcher shapefileMatcher = new ShapefileMatcher(network.streetLayer); try { diff --git a/src/main/java/com/conveyal/r5/labeling/LevelOfTrafficStressLabeler.java b/src/main/java/com/conveyal/r5/labeling/LevelOfTrafficStressLabeler.java index 76da19ebd..405ee4289 100644 --- a/src/main/java/com/conveyal/r5/labeling/LevelOfTrafficStressLabeler.java +++ b/src/main/java/com/conveyal/r5/labeling/LevelOfTrafficStressLabeler.java @@ -17,7 +17,8 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; -import static com.conveyal.r5.streets.EdgeStore.EdgeFlag.BIKE_LTS_EXPLICIT; +import static com.conveyal.r5.streets.EdgeStore.EdgeFlag.*; +import static com.conveyal.r5.streets.EdgeStore.intToLts; /** * Label streets with a best-guess at their Level of Traffic Stress, as defined in @@ -43,11 +44,20 @@ public class LevelOfTrafficStressLabeler { /** * Set the LTS for this way in the provided flags (not taking into account any intersection LTS at the moment). * This sets flags (passed in as the second and third parameters) from the tags on the OSM Way (first parameter). + * + * The general approach in this function is to look at a bunch of OSM tags (highway, cycleway, maxspeed, lanes) and + * edge characteristics (allowed modes) progressing from things implying low to high LTS, and returning early when + * lower LTS has already been established. + * + * One reason to work in order of increasing LTS is that LTS is implemented as non-mutually exclusive flags on the + * edge, and higher LTS flags override lower ones, i.e. if an LTS 4 flag is present the road is seen as LTS 4 even + * if an LTS 2 flag is present. This should really be changed, but it would be a backward-incompatible change to + * the network storage format. */ public void label (Way way, EnumSet forwardFlags, EnumSet backFlags) { - // the general idea behind this function is that we progress from low-stress to higher-stress, bailing out as we go. - // First, if the input OSM data contains LTS tags, use those rather than estimating LTS from road characteristics. + // First, if the input OSM data contains LTS tags, + // use those rather than estimating LTS from road characteristics and return immediately. String ltsTagValue = way.getTag("lts"); if (ltsTagValue != null) { try { @@ -67,36 +77,38 @@ public void label (Way way, EnumSet forwardFlags, EnumSet forwardFlags, EnumSet forwardFlags, EnumSet 4) { - LOG.error("Clamping LTS value to range [1...4]. Value in attribute is {}", lts); + LOG.error("LTS should be in range [1...4]. Value in attribute is {}", lts); } - EdgeStore.EdgeFlag ltsFlag = intToLts(lts); edge.setFlag(BIKE_LTS_EXPLICIT); - edge.setFlag(ltsFlag); + edge.setLts(lts); edge.advance(); edge.setFlag(BIKE_LTS_EXPLICIT); - edge.setFlag(ltsFlag); + edge.setLts(lts); edgePairCounter.increment(); } }); diff --git a/src/main/java/com/conveyal/r5/streets/EdgeStore.java b/src/main/java/com/conveyal/r5/streets/EdgeStore.java index 9ed498537..bc2719c58 100644 --- a/src/main/java/com/conveyal/r5/streets/EdgeStore.java +++ b/src/main/java/com/conveyal/r5/streets/EdgeStore.java @@ -14,6 +14,7 @@ import com.conveyal.r5.util.P2; import com.conveyal.r5.util.TIntIntHashMultimap; import com.conveyal.r5.util.TIntIntMultimap; +import com.google.common.base.Preconditions; import gnu.trove.iterator.TIntIntIterator; import gnu.trove.list.TByteList; import gnu.trove.list.TIntList; @@ -43,6 +44,8 @@ import java.util.StringJoiner; import java.util.function.IntConsumer; +import static com.conveyal.r5.streets.EdgeStore.EdgeFlag.*; + /** * This stores all the characteristics of the edges in the street graph layer of the transport network. * The naive way to do this is making one object per edge, and storing all those edges in one big list or in outgoing @@ -1093,6 +1096,7 @@ public void allowAllModes() { /** * Set the flags for all on-street modes of transportation to "false", so no modes can traverse this edge. + * This can be used in preparation for setting only a single mode or specific subset of modes. */ public void disallowAllModes() { clearFlag(EdgeFlag.ALLOWS_PEDESTRIAN); @@ -1101,6 +1105,26 @@ public void disallowAllModes() { clearFlag(EdgeFlag.ALLOWS_WHEELCHAIR); } + /** + * Remove all LTS flags so the edge has no LTS information. This can be done in preparation for setting one + * specific LTS on the edge. This is particularly important because LTS categories should be mutually exclusive + * but flags are not. In practice higher LTS levels override lower ones when present on the same edge, but this + * can be confusing so ideally we'll set only one flag on any given edge. + */ + public void removeAllLtsFlags() { + clearFlag(EdgeFlag.BIKE_LTS_1); + clearFlag(EdgeFlag.BIKE_LTS_2); + clearFlag(EdgeFlag.BIKE_LTS_3); + clearFlag(EdgeFlag.BIKE_LTS_4); + } + + /** Clear all LTS flags, then set the single LTS flag for the supplied level from 1 to 4. */ + public void setLts(int lts) { + Preconditions.checkArgument((lts >= 1 || lts <= 4), "LTS must be an integer in the range [1...4]."); + removeAllLtsFlags(); + setFlag(intToLts(lts)); + } + public boolean allowsStreetMode(StreetMode mode) { if (mode == StreetMode.WALK) { return getFlag(EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN); @@ -1175,6 +1199,14 @@ public Map attributesForDisplay () { } + /** Clamp the integer value to the range [1...4] and return the equivalent LTS bit flag enum value. */ + public static EdgeFlag intToLts (int lts) { + if (lts < 2) return BIKE_LTS_1; + else if (lts == 2) return BIKE_LTS_2; + else if (lts == 3) return BIKE_LTS_3; + else return BIKE_LTS_4; + } + /** * @return an Edge object that can be moved around in the EdgeStore to get a view of any edge in the graph. * This object is not pointing at any edge upon creation, so you'll need to call seek() on it to select an edge. diff --git a/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java b/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java index 9da5336b5..c9c038047 100644 --- a/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java +++ b/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java @@ -1,24 +1,28 @@ package com.conveyal.r5.analyst.scenario; +import com.conveyal.osmlib.OSM; import com.conveyal.r5.analyst.WebMercatorExtents; import com.conveyal.r5.analyst.WebMercatorGridPointSet; import com.conveyal.r5.analyst.cluster.TravelTimeSurfaceTask; +import com.conveyal.r5.analyst.scenario.FakeGraph.TransitNetwork; 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.EdgeStore.Edge; +import com.conveyal.r5.streets.EdgeStore.EdgeFlag; +import com.conveyal.r5.streets.StreetLayer; 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 com.conveyal.r5.analyst.scenario.FakeGraph.buildNetwork; +import static com.conveyal.r5.streets.EdgeStore.intToLts; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -26,18 +30,15 @@ */ public class ModifyStreetsTest { - final double FROM_LAT = 40.0; - final double FROM_LON = -83.0; - final int TIME_LIMIT_SECONDS = 1200; + private static final double FROM_LAT = 40.0; + private static final double FROM_LON = -83.0; + private static final double SIZE_DEGREES = 0.05; + private static 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; + private static final int MIN_ELEMENTS = 100; /** - * Using a `ModifyStreets` modification, test that adjusting the `walkTimeFactor` appropriately changes route time. + * Using a `ModifyStreets` modification, test that adjusting the `walkTimeFactor` appropriately changes travel time. * Also indirectly tests that the necessary generalized cost data tables will be added to the network when missing. */ @Test @@ -47,19 +48,11 @@ public void testGeneralizedCostWalk() { 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); + ms.polygons = makeModificationPolygon(FROM_LON, FROM_LAT, SIZE_DEGREES); + var modifiedNetwork = applySingleModification(network, ms); - var reachedVertices = getReachedVertices(network, new Scenario()); - var reachedVerticesWithModification = getReachedVertices(network, scenario); + var reachedVertices = getReachedVertices(network); + var reachedVerticesWithModification = getReachedVertices(modifiedNetwork); int nReachedVertices = reachedVertices.size(); int nReachedVerticesWithModification = reachedVerticesWithModification.size(); @@ -67,26 +60,21 @@ public void testGeneralizedCostWalk() { assertTrue(nReachedVertices > 500); assertTrue(nReachedVerticesWithModification > nReachedVertices * 2); - int[] nLowerTimes = new int[1]; // Sidestep annoying Java "effectively final" rule. - reachedVertices.forEachKey(v -> { + int nLowerTimes = 0; + for (int v : reachedVertices.keys()) { int travelTime = reachedVertices.get(v); int travelTimeWithModification = reachedVerticesWithModification.get(v); assertTrue(travelTimeWithModification != -1); assertTrue(travelTimeWithModification <= travelTime); if (travelTimeWithModification != travelTime) { - nLowerTimes[0] += 1; + nLowerTimes += 1; assertTrue(travelTimeWithModification * 1.3 < travelTime); } - return true; - }); - assertTrue(nLowerTimes[0] > 500); + } + assertTrue(nLowerTimes > 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); + private static TIntIntMap getReachedVertices(TransportNetwork network) { var task = new TravelTimeSurfaceTask(); task.accessModes = EnumSet.of(LegMode.WALK); task.directModes = EnumSet.of(LegMode.WALK); @@ -94,13 +82,15 @@ TIntIntMap getReachedVertices(TransportNetwork network, Scenario scenario) { 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); + WebMercatorGridPointSet gps = new WebMercatorGridPointSet(network); + task.north = gps.north; + task.west = gps.west; + task.height = gps.height; + task.width = gps.width; + task.zoom = gps.zoom; + + var streetRouter = new StreetRouter(network.streetLayer); streetRouter.profileRequest = task; streetRouter.setOrigin(task.fromLat, task.fromLon); streetRouter.streetMode = StreetMode.WALK; @@ -109,4 +99,90 @@ TIntIntMap getReachedVertices(TransportNetwork network, Scenario scenario) { return streetRouter.getReachedVertices(); } + + /** + * Test that our LTS labeling process, as well as street modifications, do not set more than one LTS value on a + * single edge. The LTS is stored as bit flags, so even though LTS values are mutually exclusive it is technically + * possible (but meaningless) for more than one to be present on the same edge. + */ + @Test + public void testLtsRepresentation () { + TransportNetwork network = buildNetwork(TransitNetwork.SINGLE_LINE); + checkStreetLayerLts(network.streetLayer); + + ModifyStreets mod = new ModifyStreets(); + mod.allowedModes = EnumSet.of(StreetMode.BICYCLE); + mod.polygons = makeModificationPolygon(FROM_LON, FROM_LAT, SIZE_DEGREES); + mod.bikeLts = 1; + + TransportNetwork modifiedNetwork = applySingleModification(network, mod); + checkStreetLayerLts(modifiedNetwork.streetLayer); + + // Check that the modification set a bunch of edges to have LTS 1. + int nLtsOneBefore = countLts(network.streetLayer, 1); + int nLtsOneAfter = countLts(modifiedNetwork.streetLayer, 1); + assertTrue(nLtsOneAfter > nLtsOneBefore * 1.5); + assertTrue(nLtsOneAfter > MIN_ELEMENTS); + } + + private void checkStreetLayerLts(StreetLayer streets) { + assertTrue(streets.edgeStore.nEdges() > MIN_ELEMENTS); + int nWithLts = 0; + for (Edge e = streets.edgeStore.getCursor(0); e.advance(); ) { + int nSet = 0; + if (e.getFlag(EdgeFlag.BIKE_LTS_1)) nSet += 1; + if (e.getFlag(EdgeFlag.BIKE_LTS_2)) nSet += 1; + if (e.getFlag(EdgeFlag.BIKE_LTS_3)) nSet += 1; + if (e.getFlag(EdgeFlag.BIKE_LTS_4)) nSet += 1; + assertTrue(nSet == 0 || nSet == 1); + if (nSet > 0) nWithLts += 1; + } + assertTrue(nWithLts > MIN_ELEMENTS); + } + + /** @return the number of edges in the supplied street layer that have the given LTS level set. **/ + private int countLts (StreetLayer streets, int lts) { + EdgeFlag ltsFlag = intToLts(lts); + int n = 0; + for (Edge e = streets.edgeStore.getCursor(0); e.advance(); ) { + if (e.getFlag(ltsFlag)) { + n += 1; + } + } + return n; + } + + //// Static utility functions for constructing test modifications and scenarios + + private static TransportNetwork applySingleModification (TransportNetwork baseNetwork, Modification modification) { + Scenario scenario = new Scenario(); + scenario.modifications = Arrays.asList(modification); + TransportNetwork modifiedNetwork = scenario.applyToTransportNetwork(baseNetwork); + Assertions.assertEquals(0, modifiedNetwork.scenarioApplicationWarnings.size()); + return modifiedNetwork; + } + + private static double[][][] makeModificationPolygon (Envelope env) { + return new double[][][]{{ + {env.getMinX(), env.getMaxY()}, + {env.getMaxX(), env.getMaxY()}, + {env.getMaxX(), env.getMinY()}, + {env.getMinX(), env.getMinY()}, + {env.getMinX(), env.getMaxY()} + }}; + } + + private static double[][][] makeModificationPolygon (double centerLon, double centerLat, double radiusDegrees) { + Envelope env = makeEnvelope(centerLon, centerLat, radiusDegrees); + return makeModificationPolygon(env); + } + + private static Envelope makeEnvelope (double centerLon, double centerLat, double radiusDegrees) { + final double west = centerLon - radiusDegrees; + final double east = centerLon + radiusDegrees; + final double south = centerLat - radiusDegrees; + final double north = centerLat + radiusDegrees; + return new Envelope(west, east, south, north); + } + } \ No newline at end of file diff --git a/src/test/resources/manual/README.txt b/src/test/resources/manual/README.txt new file mode 100644 index 000000000..9a5539cc4 --- /dev/null +++ b/src/test/resources/manual/README.txt @@ -0,0 +1,3 @@ +This directory contains fixtures for manual testing. This includes files that are useful +for improvised testing (various transportation network inputs or geospatial inputs) as well +as files referenced by well-defined manual tests carried out before each release. \ No newline at end of file From 9abdbbf34cab12c5b7424011c0b6e030ab29328f Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 21 Dec 2022 18:42:24 +0800 Subject: [PATCH 22/46] limit number of error/warning/info messages (#845) Messages are added via a method only when set is below a certain size. If a set reaches the maximum size, a message is added explaining that some messages were omitted for brevity. --- .../conveyal/r5/analyst/error/TaskError.java | 3 + .../r5/analyst/scenario/AddStreets.java | 28 ++++---- .../r5/analyst/scenario/AddTrips.java | 8 +-- .../r5/analyst/scenario/AdjustDwellTime.java | 10 +-- .../r5/analyst/scenario/AdjustFrequency.java | 12 ++-- .../r5/analyst/scenario/AdjustSpeed.java | 20 +++--- .../r5/analyst/scenario/Modification.java | 64 +++++++++++++++---- .../r5/analyst/scenario/ModifyStreets.java | 28 ++++---- .../r5/analyst/scenario/PickupDelay.java | 18 +++--- .../r5/analyst/scenario/RasterCost.java | 8 +-- .../r5/analyst/scenario/RemoveStops.java | 12 ++-- .../r5/analyst/scenario/RemoveTrips.java | 4 +- .../conveyal/r5/analyst/scenario/Reroute.java | 22 +++---- .../r5/analyst/scenario/RoadCongestion.java | 18 +++--- .../r5/analyst/scenario/ShapefileLts.java | 8 +-- .../r5/analyst/scenario/StopSpec.java | 9 +-- .../analyst/scenario/ModifyStreetsTest.java | 1 + 17 files changed, 160 insertions(+), 113 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/error/TaskError.java b/src/main/java/com/conveyal/r5/analyst/error/TaskError.java index 4019fe68e..c49cf18e8 100644 --- a/src/main/java/com/conveyal/r5/analyst/error/TaskError.java +++ b/src/main/java/com/conveyal/r5/analyst/error/TaskError.java @@ -7,6 +7,8 @@ import java.util.Collection; import java.util.List; +import static com.google.common.base.Preconditions.checkArgument; + /** * This is an API model object for reporting a single error or warning that occurred on a worker back to the UI via * the backend. The most common errors a user will see are problems applying scenario modifications, so this provides @@ -35,6 +37,7 @@ public TaskError(Throwable throwable) { public TaskError(Modification modification, Collection messages) { this.modificationId = modification.comment; this.title = "while applying the modification entitled '" + modification.comment + "'."; + checkArgument(messages.size() <= Modification.MAX_MESSAGES + 1); this.messages.addAll(messages); } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/AddStreets.java b/src/main/java/com/conveyal/r5/analyst/scenario/AddStreets.java index ef95ebf25..d0798f32b 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/AddStreets.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/AddStreets.java @@ -70,24 +70,24 @@ public class AddStreets extends Modification { @Override public boolean resolve (TransportNetwork network) { if (allowedModes == null || allowedModes.isEmpty()) { - errors.add("You must supply at least one StreetMode."); + addError("You must supply at least one StreetMode."); } if (allowedModes != null && allowedModes.contains(StreetMode.CAR)) { if (carSpeedKph == null) { - errors.add("You must supply a car speed when specifying the CAR mode."); + addError("You must supply a car speed when specifying the CAR mode."); } } if (carSpeedKph != null) { // TODO factor out repetitive range checking code into a utility function if (carSpeedKph < 1 || carSpeedKph > 150) { - errors.add("Car speed should be in the range [1...150] kph. Specified speed is: " + carSpeedKph); + addError("Car speed should be in the range [1...150] kph. Specified speed is: " + carSpeedKph); } } if (lineStrings == null || lineStrings.length < 1) { - errors.add("Modification contained no LineStrings."); + addError("Modification contained no LineStrings."); } else for (double[][] lineString : lineStrings) { if (lineString.length < 2) { - errors.add("LineString had less than two coordinates."); + addError("LineString had less than two coordinates."); } for (double[] coord : lineString) { // Incoming coordinates use the same (x, y) axis order as JTS. @@ -97,21 +97,21 @@ 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."); + addError("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]."); + addError("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."); + addError("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]."); + addError("bikeGenCostFactor must be in the range (0...10]."); } } - return errors.size() > 0; + return hasErrors(); } @Override @@ -128,7 +128,7 @@ public boolean apply (TransportNetwork network) { // Only first and last coordinates should be linked to the network. int startLinkVertex = network.streetLayer.getOrCreateVertexNear(lat, lon, linkMode); if (startLinkVertex < 0) { - errors.add(String.format("Failed to connect first vertex to streets at (%f, %f)", lat, lon)); + addError(String.format("Failed to connect first vertex to streets at (%f, %f)", lat, lon)); } prevVertex.seek(startLinkVertex); } @@ -142,7 +142,7 @@ public boolean apply (TransportNetwork network) { // Only first and last coordinates should be linked to the network. int endLinkVertex = network.streetLayer.getOrCreateVertexNear(lat, lon, linkMode); if (endLinkVertex < 0) { - errors.add(String.format("Failed to connect last vertex to streets at (%f, %f)", lat, lon)); + addError(String.format("Failed to connect last vertex to streets at (%f, %f)", lat, lon)); } currVertex.seek(endLinkVertex); createEdgePair(prevVertex, currVertex, network.streetLayer.edgeStore); @@ -156,7 +156,7 @@ public boolean apply (TransportNetwork network) { forwardEdge.seek(forwardEdgeNumber); network.streetLayer.indexTemporaryEdgePair(forwardEdge); } - return errors.size() > 0; + return hasErrors(); } private void createEdgePair (VertexStore.Vertex previousVertex, VertexStore.Vertex currentVertex, EdgeStore edgeStore) { @@ -169,7 +169,7 @@ private void createEdgePair (VertexStore.Vertex previousVertex, VertexStore.Vert currentVertex.getLon() ); if (edgeLengthMm > Integer.MAX_VALUE) { - errors.add("Edge length in millimeters would overflow an int (was longer than 2147km)."); + addError("Edge length in millimeters would overflow an int (was longer than 2147km)."); edgeLengthMm = Integer.MAX_VALUE; } // TODO mono- or bidirectional diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/AddTrips.java b/src/main/java/com/conveyal/r5/analyst/scenario/AddTrips.java index 6d87643f9..46f77498f 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/AddTrips.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/AddTrips.java @@ -77,17 +77,17 @@ public class AddTrips extends Modification { @Override public boolean resolve (TransportNetwork network) { if (stops == null || stops.size() < 2) { - errors.add("You must provide at least two stops."); + addError("You must provide at least two stops."); } else { if (frequencies.isEmpty()) { - errors.add("This modification should include at least one timetable/frequency entry."); + addError("This modification should include at least one timetable/frequency entry."); } for (PatternTimetable entry : frequencies) { - errors.addAll(entry.validate(stops.size())); + addErrors(entry.validate(stops.size())); } intStopIds = findOrCreateStops(stops, network); } - return errors.size() > 0; + return hasErrors(); } @Override diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustDwellTime.java b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustDwellTime.java index b30de9354..0682bb117 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustDwellTime.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustDwellTime.java @@ -55,7 +55,7 @@ public boolean resolve (TransportNetwork network) { for (String stringStopId : stops) { int intStopId = network.transitLayer.indexForStopId.get(stringStopId); if (intStopId == -1) { - errors.add("Could not find a stop to adjust with GTFS ID " + stringStopId); + addError("Could not find a stop to adjust with GTFS ID " + stringStopId); } else { intStops.add(intStopId); } @@ -64,10 +64,10 @@ public boolean resolve (TransportNetwork network) { } // Not bitwise operator: non-short-circuit logical XOR. if (!((dwellSecs >= 0) ^ (scale >= 0))) { - errors.add("Dwell time or scaling factor must be specified, but not both."); + addError("Dwell time or scaling factor must be specified, but not both."); } checkIds(routes, patterns, trips, true, network); - return errors.size() > 0; + return hasErrors(); } @Override @@ -78,9 +78,9 @@ public boolean apply (TransportNetwork network) { if (nTripsAffected > 0) { LOG.info("Modified {} trips.", nTripsAffected); } else { - errors.add("This modification did not affect any trips."); + addError("This modification did not affect any trips."); } - return errors.size() > 0; + return hasErrors(); } private TripPattern processTripPattern (TripPattern originalPattern) { diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustFrequency.java b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustFrequency.java index 0cf085049..4c03f6e61 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustFrequency.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustFrequency.java @@ -74,12 +74,12 @@ public class AdjustFrequency extends Modification { @Override public boolean resolve (TransportNetwork network) { if (entries.isEmpty()) { - errors.add("This modification should include at least one timetable/frequency entry."); + addError("This modification should include at least one timetable/frequency entry."); } for (PatternTimetable entry : entries) { - errors.addAll(entry.validate(-1)); + addErrors(entry.validate(-1)); } - return errors.size() > 0; + return hasErrors(); } @Override @@ -112,12 +112,12 @@ public boolean apply(TransportNetwork network) { Set unmatchedEntries = new HashSet<>(entries); unmatchedEntries.removeAll(entriesMatched); for (PatternTimetable entry : unmatchedEntries) { - errors.add("Trip not found for ID " + entry.sourceTrip + ". Ensure a trip pattern has been selected in " + + addError("Trip not found for ID " + entry.sourceTrip + ". Ensure a trip pattern has been selected in " + "this modification."); } LOG.info("Cleared {} patterns, creating {} new trip schedules.", nPatternsCleared, nTripSchedulesCreated); - return errors.size() > 0; + return hasErrors(); } /** @@ -268,7 +268,7 @@ private TripPattern processPattern(TripPattern originalPattern) { private Service blackOutService(final TripSchedule schedule) { if (schedule.headwaySeconds != null) { - errors.add("We do not currently support retaining existing frequency entries when adjusting timetables."); + addError("We do not currently support retaining existing frequency entries when adjusting timetables."); return null; } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java index 6121c7e63..5ca44a6df 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/AdjustSpeed.java @@ -89,7 +89,7 @@ public class AdjustSpeed extends Modification { @Override public boolean resolve(TransportNetwork network) { if (scale <= 0) { - errors.add("Scaling factor must be a positive number."); + addError("Scaling factor must be a positive number."); } if (hops != null) { // De-duplicate hops. See explanation on the hops and uniqueHops fields. @@ -97,7 +97,7 @@ public boolean resolve(TransportNetwork network) { Set> uniqueHopSet = new HashSet<>(); for (String[] pair : hops) { if (pair.length != 2) { - errors.add("Hops must all have exactly two stops."); + addError("Hops must all have exactly two stops."); continue; } // Pair has equals and hashcode implementations which arrays lack @@ -117,11 +117,11 @@ public boolean resolve(TransportNetwork network) { int intFromId = network.transitLayer.indexForStopId.get(pair.a); int intToId = network.transitLayer.indexForStopId.get(pair.b); if (intFromId == -1) { - errors.add("Could not find hop origin stop " + pair.a); + addError("Could not find hop origin stop " + pair.a); continue; } if (intToId == -1) { - errors.add("Could not find hop destination stop " + pair.b); + addError("Could not find hop destination stop " + pair.b); continue; } hopFromStops.add(intFromId); @@ -129,7 +129,7 @@ public boolean resolve(TransportNetwork network) { } } checkIds(routes, patterns, trips, true, network); - return errors.size() > 0; + return hasErrors(); } @Override @@ -139,19 +139,19 @@ public boolean apply(TransportNetwork network) { .collect(Collectors.toList()); if (nTripsAffected > 0) { - info.add(String.format("Changed speed on %d trips.", nTripsAffected)); + addInfo(String.format("Changed speed on %d trips.", nTripsAffected)); } else { - errors.add("This modification did not cause any changes to the transport network."); + addError("This modification did not cause any changes to the transport network."); } if (uniqueHops != null) { for (int h = 0; h < uniqueHops.size(); h++) { if (nPatternsAffectedByHop[h] == 0) { - errors.add("No patterns were affected by hop: " + uniqueHops.get(h)); + addError("No patterns were affected by hop: " + uniqueHops.get(h)); } } - info.add("Number of patterns affected by each unique hop: " + Arrays.toString(nPatternsAffectedByHop)); + addInfo("Number of patterns affected by each unique hop: " + Arrays.toString(nPatternsAffectedByHop)); } - return errors.size() > 0; + return hasErrors(); } private TripPattern processTripPattern (TripPattern originalPattern) { diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/Modification.java b/src/main/java/com/conveyal/r5/analyst/scenario/Modification.java index 03134f637..a9c533a6e 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/Modification.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/Modification.java @@ -35,6 +35,12 @@ public abstract class Modification implements Serializable { private static final Logger LOG = LoggerFactory.getLogger(Modification.class); + /** + * The maximum number of error messages in each category (info/warning/error) to store in each modification. + * This prevents bandwidth/latency/memory problems from sending enormous lists of errors back to the client. + */ + public static final int MAX_MESSAGES = 5; + /** Free-text comment describing this modification instance and what it's intended to do or represent. */ public String comment; @@ -47,14 +53,15 @@ public abstract class Modification implements Serializable { /** * The "resolve" method is called on each Modification before it is applied. If any problems are detected, the * Modification should not be applied, and this Set should contain Strings describing all the problems. + * To limit size, always add error/warning/info messages using the addX methods rather than direct field access. */ public transient final Set errors = new HashSet<>(); /** * Any warnings that should be presented to the user but which do not prevent scenario application should appear here. + * TODO this should be transient like the other error/info fields but previously wasn't. R5 modifications are stored + * in MongoDB in regional analyses, which means that marking it transient causes deserialization headaches. */ - // TODO this should be transient as well but previously wasn't. R5 modifications are stored in MongoDB in regional - // analyses, which means that marking it transient causes deserialization headaches. public final Set warnings = new HashSet<>(); /** @@ -69,7 +76,8 @@ public abstract class Modification implements Serializable { * or other deep objects from the original TransportNetwork. The Modification is responsible for ensuring that * no damage is done to the original TransportNetwork by making copies of referenced objects as necessary. * - * TODO arguably this and resolve() don't need to return booleans - all implementations just check whether the errors list is empty, so we could just supply a method that does that. + * TODO arguably this and resolve() don't need to return booleans - all implementations just check whether the + * errors list is empty, so we could just supply a method that does that. * TODO remove the network field, and use the network against which this Modification was resolved. * @return true if any errors happened while applying the modification. */ @@ -126,7 +134,7 @@ protected TIntList findOrCreateStops(List stops, TransportNetwork netw // Create or find the stops referenced by the new trips. TIntList intStopIds = new TIntArrayList(); for (StopSpec stopSpec : stops) { - int intStopId = stopSpec.resolve(network, errors); + int intStopId = stopSpec.resolve(network, this); intStopIds.add(intStopId); } // Adding the stops changes the street network but does not rebuild the edge lists. @@ -173,14 +181,14 @@ public void checkIds (Set routes, Set patterns, Set trip } if (nDefined != 1) { if (allowTrips) { - errors.add("Exactly one of routes, patterns, or trips must be provided."); + addError("Exactly one of routes, patterns, or trips must be provided."); } else { - errors.add("Either routes or patterns must be provided, but not both."); + addError("Either routes or patterns must be provided, but not both."); } } if (!allowTrips && trips != null) { // This cannot happen yet, but it will if (TODO) we pull these fields up to the Modification class. - errors.add("This modification type does not allow specifying individual trips by ID."); + addError("This modification type does not allow specifying individual trips by ID."); } // TODO pull the network field up into the Modification class. @@ -192,18 +200,18 @@ public void checkIds (Set routes, Set patterns, Set trip } if (unmatchedRoutes.size() > 0) { if (unmatchedRoutes.size() == routes.size()) { - errors.add("None of the specified route IDs could be found."); + addError("None of the specified route IDs could be found."); } else { // When a GTFS feed contains routes that have no stoptimes (which is unfortunately common) R5 will not // represent that route. When someone bulk-adds all the routes in that feed to a modification, // some of them may be valid while others are not. // Specifying some routes that don't exist seems like a severe problem that should be an error rather // than a warning, but for now we have to tolerate it for the above reason. - warnings.add("Some of the specified route IDs could be found: " + unmatchedRoutes.toString()); + addWarning("Some of the specified route IDs could be found: " + unmatchedRoutes.toString()); } } if (unmatchedTrips.size() > 0) { - errors.add("These trip IDs could not be found: " + unmatchedTrips.toString()); + addError("These trip IDs could not be found: " + unmatchedTrips.toString()); } } @@ -223,7 +231,41 @@ protected void rangeCheckCoordinate (Coordinate coordinate, TransportNetwork net coordinate.toString(), envelope.toString() ); - errors.add(message); + addError(message); + } + } + + + // Always add messages using the methods below to limit the size of the sets and the responses to the client. + + protected void addWarning (String warning) { + addMessage(warnings, warning); + } + + protected void addError (String error) { + addMessage(errors, error); + } + + protected void addErrors (Iterable errorIterable) { + for (String error : errorIterable) { + addError(error); + } + } + + protected void addInfo (String info) { + addMessage(this.info, info); + } + + protected boolean hasErrors () { + return errors.size() > 0; + } + + private static void addMessage (Set target, String message) { + if (target.size() < MAX_MESSAGES) { + target.add(message); + } else if (target.size() == MAX_MESSAGES) { + target.add("Additional messages omitted for brevity."); } } + } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java b/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java index e436d2e8f..5be306055 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/ModifyStreets.java @@ -101,18 +101,18 @@ public boolean resolve (TransportNetwork network) { final GeometryFactory geometryFactory = Geometries.geometryFactory; List jtsPolygons = new ArrayList<>(); if (polygons == null || polygons.length == 0) { - errors.add("You must specify some polygons to select streets."); + addError("You must specify some polygons to select streets."); polygons = new double[][][]{}; } for (double[][] polygon : polygons) { if (polygon.length < 3) { - errors.add("Polygons must have at least three coordinates to enclose any space."); + addError("Polygons must have at least three coordinates to enclose any space."); continue; } List jtsCoordinates = new ArrayList<>(); for (double[] coordinate : polygon) { if (coordinate.length != 2) { - errors.add("Each coordinate must have two values, a latitude and a longitude."); + addError("Each coordinate must have two values, a latitude and a longitude."); continue; } Coordinate jtsCoordinate = new Coordinate(coordinate[0], coordinate[1]); @@ -140,43 +140,43 @@ public boolean resolve (TransportNetwork network) { } return true; }); - info.add(String.format("Will affect %d edges out of %d candidates.", edgesInPolygon.size(), + addInfo(String.format("Will affect %d edges out of %d candidates.", edgesInPolygon.size(), candidateEdges.size())); // Range check and otherwise validate numeric parameters if (carSpeedKph != null && carSpeedFactor != null) { - errors.add("You must specify only one of carSpeedKph or carSpeedFactor."); + addError("You must specify only one of carSpeedKph or carSpeedFactor."); } if (carSpeedKph != null) { if (carSpeedKph <= 0 || carSpeedKph > 130) { - errors.add("Car speed must be in the range (0...130] kph."); + addError("Car speed must be in the range (0...130] kph."); } } if (carSpeedFactor != null) { if (carSpeedFactor <= 0 || carSpeedFactor > 10) { - errors.add("Car speed factor must be in the range (0...10]."); + addError("Car speed factor must be in the range (0...10]."); } } if (walkTimeFactor != null) { if (walkTimeFactor <= 0 || walkTimeFactor > 10) { - errors.add("walkGenCostFactor must be in the range (0...10]."); + addError("walkGenCostFactor must be in the range (0...10]."); } } if (bikeTimeFactor != null) { if (bikeTimeFactor <= 0 || bikeTimeFactor > 10) { - errors.add("bikeGenCostFactor must be in the range (0...10]."); + addError("bikeGenCostFactor must be in the range (0...10]."); } } if (bikeLts != null) { if (bikeLts < 0 || bikeLts > 4) { - errors.add("bikeLts must be in the range [0...4]."); + addError("bikeLts must be in the range [0...4]."); } } if (allowedModes == null) { - errors.add("You must specify a list of allowedModes, which may be empty."); + addError("You must specify a list of allowedModes, which may be empty."); } - return errors.size() > 0; + return hasErrors(); } @Override @@ -184,7 +184,7 @@ 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."); + addInfo("Added table of per-edge factors because base network doesn't have one."); network.streetLayer.edgeStore.edgeTraversalTimes = EdgeTraversalTimes.createNeutral(network.streetLayer.edgeStore); } } @@ -221,7 +221,7 @@ public boolean apply (TransportNetwork network) { } // Instead of repeating this error logic in every resolve and apply method, // we should really be doing this using a modification.hasErrors() method from one frame up. - return errors.size() > 0; + return hasErrors(); } /** diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/PickupDelay.java b/src/main/java/com/conveyal/r5/analyst/scenario/PickupDelay.java index d04169c26..49ef81f41 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/PickupDelay.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/PickupDelay.java @@ -111,7 +111,7 @@ public boolean resolve (TransportNetwork network) { ); polygons.loadFromS3GeoJson(); // Collect any errors from the IndexedPolygonCollection construction, so they can be seen in the UI. - errors.addAll(polygons.getErrors()); + addErrors(polygons.getErrors()); // Handle pickup service to stop mapping if supplied in the modification JSON. if (stopsForZone == null) { this.pickupWaitTimes = new PickupWaitTimes(polygons, null, Collections.emptySet(), this.streetMode); @@ -122,12 +122,12 @@ public boolean resolve (TransportNetwork network) { final Map stopNumbersForZonePolygon = new HashMap<>(); final Map egressServices = new HashMap<>(); if (stopsForZone.isEmpty()) { - errors.add("If stopsForZone is specified, it must be non-empty."); + addError("If stopsForZone is specified, it must be non-empty."); } stopsForZone.forEach((zonePolygonId, stopPolygonIds) -> { ModificationPolygon zonePolygon = polygons.getById(zonePolygonId); if (zonePolygon == null) { - errors.add("Could not find zone polygon with ID: " + zonePolygonId); + addError("Could not find zone polygon with ID: " + zonePolygonId); } TIntSet stopNumbers = stopNumbersForZonePolygon.get(zonePolygon); if (stopNumbers == null) { @@ -137,11 +137,11 @@ public boolean resolve (TransportNetwork network) { for (String stopPolygonId : stopPolygonIds) { ModificationPolygon stopPolygon = polygons.getById(stopPolygonId); if (stopPolygon == null) { - errors.add("Could not find stop polygon with ID: " + stopPolygonId); + addError("Could not find stop polygon with ID: " + stopPolygonId); } TIntSet stops = network.transitLayer.findStopsInGeometry(stopPolygon.polygonal); if (stops.isEmpty()) { - errors.add("Stop polygon did not contain any stops: " + stopPolygonId); + addError("Stop polygon did not contain any stops: " + stopPolygonId); } stopNumbers.addAll(stops); // Derive egress services from this pair of polygons @@ -171,9 +171,9 @@ public boolean resolve (TransportNetwork network) { } } catch (Exception e) { // Record any unexpected errors to bubble up to the UI. - errors.add(ExceptionUtils.stackTraceString(e)); + addError(ExceptionUtils.stackTraceString(e)); } - return errors.size() > 0; + return hasErrors(); } @Override @@ -181,11 +181,11 @@ public boolean apply (TransportNetwork network) { // network.streetLayer is already a protective copy made by method Scenario.applyToTransportNetwork. // The polygons have already been validated in the resolve method, we just need to record them in the network. if (network.streetLayer.pickupWaitTimes != null) { - errors.add("Multiple pickup delay modifications cannot be applied to a single network."); + addError("Multiple pickup delay modifications cannot be applied to a single network."); } else { network.streetLayer.pickupWaitTimes = this.pickupWaitTimes; } - return errors.size() > 0; + return hasErrors(); } @Override diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/RasterCost.java b/src/main/java/com/conveyal/r5/analyst/scenario/RasterCost.java index 10bbdece0..333ec787e 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/RasterCost.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/RasterCost.java @@ -71,7 +71,7 @@ public boolean resolve (TransportNetwork network) { loader = new SunLoader(dataSourceId); } if (loader == null) { - errors.add("Unrecognized cost function: " + costFunction); + addError("Unrecognized cost function: " + costFunction); } else { loader.setNorthShiftMeters(northShiftMeters); loader.setEastShiftMeters(eastShiftMeters); @@ -80,12 +80,12 @@ public boolean resolve (TransportNetwork network) { loader.setInputScale(inputScale); loader.setOutputScale(outputScale); } - return errors.size() > 0; + return hasErrors(); } private void checkScaleRange (double scale) { if (scale <= 0 || scale >= 100) { - errors.add("Scale parameters must be positive nonzero real numbers less than 100."); + addError("Scale parameters must be positive nonzero real numbers less than 100."); } } @@ -98,7 +98,7 @@ public boolean apply (TransportNetwork network) { edgeStore.costFields = new ArrayList<>(); } edgeStore.costFields.add(costField); - return errors.size() > 0; + return hasErrors(); } @Override diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/RemoveStops.java b/src/main/java/com/conveyal/r5/analyst/scenario/RemoveStops.java index 32b5f0223..9a50d3655 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/RemoveStops.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/RemoveStops.java @@ -69,19 +69,19 @@ public boolean resolve (TransportNetwork network) { checkIds(routes, patterns, null, false, network); intStops = new TIntHashSet(); if (stops == null || stops.isEmpty()) { - errors.add("You must supply some stops to remove."); + addError("You must supply some stops to remove."); } else { for (String stringStopId : stops) { int intStopId = network.transitLayer.indexForStopId.get(stringStopId); if (intStopId == -1) { - errors.add("Could not find a stop with GTFS ID " + stringStopId); + addError("Could not find a stop with GTFS ID " + stringStopId); } else { intStops.add(intStopId); } } LOG.info("Resolved stop IDs for removal. Strings {} resolved to integers {}.", stops, intStops); } - return errors.size() > 0; // TODO make a function for this on the superclass + return hasErrors(); } @Override @@ -93,9 +93,9 @@ public boolean apply(TransportNetwork network) { if (nPatternsAffected > 0) { LOG.info("Stops were removed from {} patterns.", nPatternsAffected); } else { - errors.add("No patterns had any stops removed by this modification."); + addError("No patterns had any stops removed by this modification."); } - return errors.size() > 0; + return hasErrors(); } public TripPattern processTripPattern (TripPattern originalTripPattern, TransportNetwork network) { @@ -221,7 +221,7 @@ private void generateNegativeTravelTimeWarning(int[] problemStops, String tripId secondsWeWillActuallyRemovePerStop ); - warnings.add(warning); + addWarning(warning); LOG.info(warning); } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/RemoveTrips.java b/src/main/java/com/conveyal/r5/analyst/scenario/RemoveTrips.java index 3d03a60ad..edc82dee2 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/RemoveTrips.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/RemoveTrips.java @@ -35,7 +35,7 @@ public class RemoveTrips extends Modification { public boolean resolve (TransportNetwork network) { int nDefined = 0; checkIds(routes, patterns, trips, true, network); - return errors.size() > 0; + return hasErrors(); } @Override @@ -62,7 +62,7 @@ public boolean apply(TransportNetwork network) { int nPatternsRemoved = nPatternsBefore - transitLayer.tripPatterns.size(); LOG.info("Removed {} entire patterns. Removed {} individual trips specified by ID.", nPatternsRemoved, nTripsRemoved); if (nTripsRemoved == 0 && nPatternsRemoved == 0) { - errors.add("No trips were removed."); + addError("No trips were removed."); } return false; } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/Reroute.java b/src/main/java/com/conveyal/r5/analyst/scenario/Reroute.java index d1fef08d2..20ba991bb 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/Reroute.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/Reroute.java @@ -104,18 +104,18 @@ NOTE These variables will be changing while apply() is on the stack. public boolean resolve (TransportNetwork network) { checkIds(routes, patterns, null, false, network); if (fromStop == null && toStop == null) { - errors.add("At least one of from and to stop must be supplied."); + addError("At least one of from and to stop must be supplied."); } if (fromStop != null) { intFromStop = network.transitLayer.indexForStopId.get(fromStop); if (intFromStop == -1) { - errors.add("Could not find fromStop with GTFS ID " + fromStop); + addError("Could not find fromStop with GTFS ID " + fromStop); } } if (toStop != null) { intToStop = network.transitLayer.indexForStopId.get(toStop); if (intToStop == -1) { - errors.add("Could not find toStop with GTFS ID " + toStop); + addError("Could not find toStop with GTFS ID " + toStop); } } // It is fine to add no new stops, @@ -134,15 +134,15 @@ public boolean resolve (TransportNetwork network) { dwellTimes = new int[0]; } if (dwellTimes.length != expectedDwells) { - errors.add("You must supply one dwell time per new stop, plus one for fromStop and one for toStop when they are specified."); + addError("You must supply one dwell time per new stop, plus one for fromStop and one for toStop when they are specified."); } if (hopTimes == null) { - errors.add("You must always supply some hop times."); + addError("You must always supply some hop times."); } else if (hopTimes.length != expectedDwells - 1) { - errors.add("The number of hops must always be one less than the number of dwells."); + addError("The number of hops must always be one less than the number of dwells."); } intNewStops = findOrCreateStops(stops, network); - return errors.size() > 0; + return hasErrors(); } @Override @@ -154,9 +154,9 @@ public boolean apply(TransportNetwork network) { if (nPatternsAffected > 0) { LOG.info("Rerouted {} patterns.", nPatternsAffected); } else { - errors.add("No patterns were rerouted."); + addError("No patterns were rerouted."); } - return errors.size() > 0; + return hasErrors(); } private TransportNetwork network; @@ -207,12 +207,12 @@ private TripPattern processTripPattern (TripPattern originalTripPattern) { } else { // This Modification is being applied to specifically chosen patterns. // If one does not contain the from or to stops, it's probably a badly formed modification so fail hard. - errors.add(warning); + addError(warning); } return originalTripPattern; } if (insertEndIndex < insertBeginIndex) { - errors.add("The end of the insertion region must be at or after its beginning."); + addError("The end of the insertion region must be at or after its beginning."); return originalTripPattern; } diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/RoadCongestion.java b/src/main/java/com/conveyal/r5/analyst/scenario/RoadCongestion.java index 86d048c49..e359bcaaf 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/RoadCongestion.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/RoadCongestion.java @@ -122,7 +122,7 @@ public boolean resolve (TransportNetwork network) { // so it's better to just remove the CRS from all input files. CoordinateReferenceSystem crs = featureType.getCoordinateReferenceSystem(); if (crs != null && !DefaultGeographicCRS.WGS84.equals(crs) && !CRS.decode("CRS:84").equals(crs)) { - errors.add("GeoJSON should specify no coordinate reference system, and contain unprojected WGS84 " + + addError("GeoJSON should specify no coordinate reference system, and contain unprojected WGS84 " + "coordinates. CRS is: " + crs.toString()); } // PropertyDescriptor scalingPropertyDescriptor = featureType.getDescriptor(scalingAttribute); @@ -142,24 +142,24 @@ public boolean resolve (TransportNetwork network) { if (name == null) { logUpdatedEdgeCounts = false; } else if (!(name instanceof String)) { - errors.add(String.format("Value '%s' of attribute '%s' of feature %d should be a string.", + addError(String.format("Value '%s' of attribute '%s' of feature %d should be a string.", name, nameAttribute, featureNumber)); indexThisFeature = false; } if (priority == null) { priority = 0; } else if (!(priority instanceof Number)) { - errors.add(String.format("Value '%s' of attribute '%s' of feature %d should be a number.", + addError(String.format("Value '%s' of attribute '%s' of feature %d should be a number.", priority, priorityAttribute, featureNumber)); indexThisFeature = false; } if (!(scale instanceof Number)) { - errors.add(String.format("Value '%s' of attribute '%s' of feature %d should be a number.", + addError(String.format("Value '%s' of attribute '%s' of feature %d should be a number.", scale, scalingAttribute, featureNumber)); indexThisFeature = false; } if (!(geometry instanceof Polygonal)) { - errors.add(String.format("Geometry of feature %d should be a Polygon or Multipolygon.", featureNumber)); + addError(String.format("Geometry of feature %d should be a Polygon or Multipolygon.", featureNumber)); indexThisFeature = false; } if (indexThisFeature) { @@ -176,9 +176,9 @@ public boolean resolve (TransportNetwork network) { logUpdatedEdgeCounts = false; } } catch (Exception e) { - errors.add(ExceptionUtils.stackTraceString(e)); + addError(ExceptionUtils.stackTraceString(e)); } - return errors.size() > 0; + return hasErrors(); } /** @@ -239,13 +239,13 @@ public boolean apply (TransportNetwork network) { } if (logUpdatedEdgeCounts) { edgeCounts.forEachEntry((polygon, quantity) -> { - info.add(String.format("polygon '%s' scaled %d edges by %.2f", polygon.name, quantity, polygon.scale)); + addInfo(String.format("polygon '%s' scaled %d edges by %.2f", polygon.name, quantity, polygon.scale)); // LOG.info("{} edges were scaled by {} via polygon {} ", quantity, polygon.scale, polygon.name); return true; }); } edgeStore.speeds = adjustedSpeeds; - return errors.size() > 0; + return hasErrors(); } @Override diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java b/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java index e39163e29..10279b002 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/ShapefileLts.java @@ -44,12 +44,12 @@ public boolean resolve (TransportNetwork network) { try { TransportNetworkCache.prefetchShapefile(WorkerComponents.fileStorage, dataSourceId); } catch (DataSourceException dx) { - errors.add(ExceptionUtils.shortAndLongString(dx)); + addError(ExceptionUtils.shortAndLongString(dx)); return true; } fileStorageKey = new FileStorageKey(DATASOURCES, dataSourceId, FileStorageFormat.SHP.extension); localFile = WorkerComponents.fileStorage.getFile(fileStorageKey); - return errors.size() > 0; + return hasErrors(); } @Override @@ -63,9 +63,9 @@ public boolean apply (TransportNetwork network) { try { shapefileMatcher.match(localFile.getAbsolutePath(), ltsAttribute); } catch (Exception e) { - errors.add(ExceptionUtils.shortAndLongString(e)); + addError(ExceptionUtils.shortAndLongString(e)); } - return errors.size() > 0; + return hasErrors(); } @Override diff --git a/src/main/java/com/conveyal/r5/analyst/scenario/StopSpec.java b/src/main/java/com/conveyal/r5/analyst/scenario/StopSpec.java index d389448c7..4a08d4994 100644 --- a/src/main/java/com/conveyal/r5/analyst/scenario/StopSpec.java +++ b/src/main/java/com/conveyal/r5/analyst/scenario/StopSpec.java @@ -52,24 +52,25 @@ public StopSpec () { * TODO maybe make protective copies a subclass of Networks to enforce this typing. * * @param network the transit network in which to find or create the specified transit stops. + * @param modification the modification in which to store any validation errors. * @return the integer index of the new stop within the given network, or -1 if it could not be created. */ - public int resolve (TransportNetwork network, Set warnings) { + public int resolve (TransportNetwork network, Modification modification) { if (id == null) { // No stop ID supplied, this is a new stop rather than a reference to an existing one. if (lat == 0 || lon == 0) { - warnings.add("When no stop ID is supplied, nonzero coordinates must be supplied."); + modification.addError("When no stop ID is supplied, nonzero coordinates must be supplied."); } int newStopId = materializeOne(network); return newStopId; } else { // Stop ID supplied, this is a reference to an existing stop rather than a new stop. if (lat != 0 || lon != 0 || name != null) { - warnings.add("A reference to an existing id should not include coordinates or a name."); + modification.addError("A reference to an existing id should not include coordinates or a name."); } int intStopId = network.transitLayer.indexForStopId.get(id); if (intStopId == -1) { - warnings.add("Could not find existing stop with GTFS ID " + id); + modification.addError("Could not find existing stop with GTFS ID " + id); } return intStopId; } diff --git a/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java b/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java index c9c038047..29b85c54c 100644 --- a/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java +++ b/src/test/java/com/conveyal/r5/analyst/scenario/ModifyStreetsTest.java @@ -152,6 +152,7 @@ private int countLts (StreetLayer streets, int lts) { return n; } + //// Static utility functions for constructing test modifications and scenarios private static TransportNetwork applySingleModification (TransportNetwork baseNetwork, Modification modification) { From 711bdb771d8dbe23be84e0cf8a3010a9df080556 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 22 Dec 2022 10:06:50 +0800 Subject: [PATCH 23/46] Disallow travel through barrier nodes (#841) fixes a problem where underground platforms at Delft station were not connected to the station stairs, but were connected to an emergency exit at the far end of the platform. Works by creating multiple vertices for barrier nodes, one for each time the node is referenced by a way. add tests and factor out some reusable methods add flag for impassable vertices add method to set flag on vertex without creating cursor object detect and split non-intersection impassable nodes Co-authored-by: ansons --- .../com/conveyal/r5/streets/StreetLayer.java | 71 +++++++++++++++--- .../com/conveyal/r5/streets/VertexStore.java | 15 +++- .../conveyal/r5/streets/StreetLayerTest.java | 69 ++++++++++++++--- .../com/conveyal/r5/streets/delft-station.pbf | Bin 0 -> 17046 bytes 4 files changed, 132 insertions(+), 23 deletions(-) create mode 100644 src/test/resources/com/conveyal/r5/streets/delft-station.pbf diff --git a/src/main/java/com/conveyal/r5/streets/StreetLayer.java b/src/main/java/com/conveyal/r5/streets/StreetLayer.java index 87e176c64..6864aadca 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetLayer.java +++ b/src/main/java/com/conveyal/r5/streets/StreetLayer.java @@ -20,6 +20,7 @@ import com.conveyal.r5.point_to_point.builder.SpeedConfig; import com.conveyal.r5.profile.StreetMode; import com.conveyal.r5.streets.EdgeStore.Edge; +import com.conveyal.r5.streets.VertexStore.VertexFlag; import com.conveyal.r5.transit.TransitLayer; import com.conveyal.r5.transit.TransportNetwork; import com.conveyal.r5.util.P2; @@ -54,6 +55,8 @@ import static com.conveyal.r5.analyst.scenario.PickupWaitTimes.NO_WAIT_ALL_STOPS; import static com.conveyal.r5.common.GeometryUtils.checkWgsEnvelopeSize; +import static com.conveyal.r5.streets.VertexStore.VertexFlag.IMPASSABLE; +import static com.conveyal.r5.streets.VertexStore.VertexFlag.TRAFFIC_SIGNAL; /** * This class stores the street network. Information about public transit is in a separate layer. @@ -143,6 +146,13 @@ public class StreetLayer implements Serializable, Cloneable { /** Envelope of this street layer, in decimal degrees (floating, not fixed-point) */ public Envelope envelope = new Envelope(); + /** + * Map from OSM node ID to internal vertex ID, which is built up as vertices are created. + * For almost all nodes there is a one-to-one mapping from OSM nodes to vertices. In special cases such as + * impassable barrier nodes, a node may be split into multiple vertices, one for each time it is referenced. + * Only the most recently created vertex for such split nodes is included in this map, which means they may not + * behave properly for turn restriction purposes. + */ TLongIntMap vertexIndexForOsmNode = new TLongIntHashMap(100_000, 0.75f, -1, -1); // Initialize these when we have an estimate of the number of expected edges. @@ -294,10 +304,16 @@ void loadFromOsm (OSM osm, boolean removeIslands, boolean saveVertexIndex) { if (!isWayRoutable(way)) { continue; } - int beginIdx = 0; // Break each OSM way into topological segments between intersections, and make one edge pair per segment. + // This is accessing every node and its tags as we process the ways. However we don't expect this + // to affect performance because the same sequence of nodes is accessed just afterward in makeEdgePair. + int beginIdx = 0; for (int n = 1; n < way.nodes.length; n++) { - if (osm.intersectionNodes.contains(way.nodes[n]) || n == (way.nodes.length - 1)) { + long nodeId = way.nodes[n]; + Node node = osm.nodes.get(nodeId); + final boolean intersection = osm.intersectionNodes.contains(way.nodes[n]); + final boolean lastNode = (n == (way.nodes.length - 1)); + if (intersection || lastNode || isImpassable(node)) { makeEdgePair(way, beginIdx, n, entry.getKey()); beginIdx = n; } @@ -1004,21 +1020,27 @@ void addReverseTurnRestriction(TurnRestriction turnRestriction, int index) { /** * Get or create mapping from a global long OSM ID to an internal street vertex ID, creating the vertex as needed. + * Generally we produce only one vertex per OSM node, but in special cases such as impassable barriers we may create + * more than one. In those cases, only the last one to be created will appear in vertexIndexForOsmNode. * @return the internal ID for the street vertex that was found or created, or -1 if there was no such OSM node. */ private int getVertexIndexForOsmNode(long osmNodeId) { int vertexIndex = vertexIndexForOsmNode.get(osmNodeId); - if (vertexIndex == -1) { - // Register a new vertex, incrementing the index starting from zero. - // Store node coordinates for this new street vertex + if (vertexIndex == -1 || vertexStore.getFlag(vertexIndex, IMPASSABLE)) { Node node = osm.nodes.get(osmNodeId); if (node == null) { - LOG.warn("OSM data references an undefined node. This is often the result of extracting a bounding box in Osmosis without the completeWays option."); + LOG.warn("OSM data references an undefined node. " + + "This is often the result of extracting a bounding box in Osmosis without the completeWays option."); + return -1; } else { + // Register a new vertex with the OSM node's coords, assigning sequential vertex IDs starting from zero. vertexIndex = vertexStore.addVertex(node.getLat(), node.getLon()); - VertexStore.Vertex v = vertexStore.getCursor(vertexIndex); - if (node.hasTag("highway", "traffic_signals")) - v.setFlag(VertexStore.VertexFlag.TRAFFIC_SIGNAL); + if (node.hasTag("highway", "traffic_signals")) { + vertexStore.setFlag(vertexIndex, TRAFFIC_SIGNAL); + } + if (isImpassable(node)) { + vertexStore.setFlag(vertexIndex, IMPASSABLE); + } vertexIndexForOsmNode.put(osmNodeId, vertexIndex); } } @@ -1048,6 +1070,27 @@ private static short speedToShort(Float speed) { return (short) Math.round(speed * 100); } + /** + * Return whether this node can be traversed. This is particularly important for nodes that connect one part of the + * network to another. For example, an emergency exit connecting station platforms to an outside path. + * In the event of poor connectivity in the input OSM data, we want such platforms to be identified + * as disconnected so they will be removed, rather than staying connected via a locked door. + * + * At first it seems like we'd want to detect nodes with barrier=* and treat them as impassable. However, looking at + * https://wiki.openstreetmap.org/wiki/Key:barrier#Values we see that most of the values represent things that are + * easily passable, and even barrier=gate is implicitly openable unless tagged with access=no|private. + * + * This code is hit millions of times so we want to bypass it as much as possible. + * If tags are present, memoize the values rather than repeatedly extracting them with hasTag(). + */ + private static boolean isImpassable (Node node) { + if (node.hasNoTags()) { + return false; + } + String access = node.getTag("access"); + return node.hasTag("entrance", "emergency") || "no".equals(access) || "private".equals(access); + } + /** * Make an edge for a sub-section of an OSM way, typically between two intersections or leading up to a dead end. */ @@ -1056,7 +1099,7 @@ private void makeEdgePair (Way way, int beginIdx, int endIdx, Long osmID) { long beginOsmNodeId = way.nodes[beginIdx]; long endOsmNodeId = way.nodes[endIdx]; - // Will create mapping if it doesn't exist yet. + // Will create a vertex for the OSM node if one doesn't exist yet. int beginVertexIndex = getVertexIndexForOsmNode(beginOsmNodeId); int endVertexIndex = getVertexIndexForOsmNode(endOsmNodeId); @@ -1638,6 +1681,14 @@ public boolean edgeIsAddedByScenario (int p) { return this.isScenarioCopy() && p >= edgeStore.firstModifiableEdge; } + /** @return whether a given flag is set to a given boolean value for all incoming and outgoing edges at a vertex. */ + public boolean flagsAroundVertex(int v, EdgeStore.EdgeFlag flag, boolean flagSet) { + return Arrays.stream(incomingEdges.get(v).toArray()) + .allMatch(i -> edgeStore.getCursor(i).getFlag(flag) == flagSet) && + Arrays.stream(outgoingEdges.get(v).toArray()) + .allMatch(i -> edgeStore.getCursor(i).getFlag(flag) == flagSet); + } + @Override public String toString() { String detail = "(base)"; diff --git a/src/main/java/com/conveyal/r5/streets/VertexStore.java b/src/main/java/com/conveyal/r5/streets/VertexStore.java index 07561cf9d..ffb4efb8a 100644 --- a/src/main/java/com/conveyal/r5/streets/VertexStore.java +++ b/src/main/java/com/conveyal/r5/streets/VertexStore.java @@ -94,11 +94,11 @@ public void setLon(double lon) { } public boolean getFlag(VertexFlag flag) { - return (vertexFlags.get(index) & flag.flag) != 0; + return VertexStore.this.getFlag(index, flag); } public void setFlag(VertexFlag flag) { - vertexFlags.set(index, (byte)(vertexFlags.get(index) | flag.flag)); + VertexStore.this.setFlag(index, flag); } public double getLat() { @@ -178,11 +178,20 @@ public static Envelope envelopeToFixed(Envelope env) { ); } + public boolean getFlag (int index, VertexFlag flag) { + return (vertexFlags.get(index) & flag.flag) != 0; + } + + public void setFlag (int index, VertexFlag flag) { + vertexFlags.set(index, (byte)(vertexFlags.get(index) | flag.flag)); + } + public enum VertexFlag { TRAFFIC_SIGNAL(0), // This intersection has a traffic signal PARK_AND_RIDE(1), - BIKE_SHARING(2); + BIKE_SHARING(2), + IMPASSABLE(3); // Nodes like emergency exits and locked gates. See StreetLayer.isImpassable. // TODO Add Stop and split vertex flags. This allows blocking U-turns at splits and eliminating the special stop finder visitor diff --git a/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java b/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java index b62b183cf..a062bec61 100644 --- a/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java +++ b/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java @@ -2,6 +2,7 @@ import com.conveyal.osmlib.OSM; import com.conveyal.r5.profile.StreetMode; +import com.conveyal.r5.streets.VertexStore.VertexFlag; import gnu.trove.TIntCollection; import gnu.trove.iterator.TIntIterator; import gnu.trove.list.TIntList; @@ -10,7 +11,6 @@ import org.locationtech.jts.geom.Coordinate; import java.util.ArrayList; -import java.util.Arrays; import java.util.EnumSet; import java.util.List; @@ -40,10 +40,7 @@ public void testSubgraphRemoval () { assertEquals(3, sl.outgoingEdges.get(v).size()); // make sure that it's a subgraph - StreetRouter r = new StreetRouter(sl); - r.setOrigin(v); - r.route(); - assertTrue(r.getReachedVertices().size() < 40); + assertTrue(connectedVertices(sl, v) < 40); int e0 = sl.incomingEdges.get(v).get(0); int e1 = e0 % 2 == 0 ? e0 + 1 : e0 - 1; @@ -54,11 +51,7 @@ public void testSubgraphRemoval () { new TarjanIslandPruner(sl, 40, StreetMode.WALK).run(); // note: disconnected subgraphs are not removed, they are de-pedestrianized - final EdgeStore.Edge edge = sl.edgeStore.getCursor(); - assertTrue(Arrays.stream(sl.incomingEdges.get(v).toArray()) - .noneMatch(i -> sl.edgeStore.getCursor(i).getFlag(EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN))); - assertTrue(Arrays.stream(sl.outgoingEdges.get(v).toArray()) - .noneMatch(i -> sl.edgeStore.getCursor(i).getFlag(EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN))); + assertTrue(sl.flagsAroundVertex(v, EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN, false)); } /** @@ -351,4 +344,60 @@ public void testComplexTurnRestriction () { assertEquals(238215856L, edge.getOSMID()); assertFalse(restriction.only); } + + /** + * Test pruning of platforms and edges that are isolated by impassable barrier nodes (e.g., emergency exits). + * The OSM is based on the Delft railway station, which has four underground platforms (numbered in increasing + * order east to west). The test fixture uses an area representation for platforms 1 and 3 and a (non-closed) + * path representation for platforms 2 and 4. The ways for platforms 1 and 2 are connected to the rest of the + * network by stairways and an elevator. + *

+ * The only connection between platforms 3 and 4 and the rest of the network is an emergency exit at their + * southern end. So we expect island pruning to clear the ALLOWS_PEDESTRIAN flag for platforms 3 and 4. + */ + @Test + public void testBarrierFiltering () { + OSM osm = new OSM(null); + osm.intersectionDetection = true; + osm.readFromUrl(StreetLayerTest.class.getResource("delft-station.pbf").toString()); + + StreetLayer sl = new StreetLayer(); + sl.loadFromOsm(osm, true, true); + sl.buildEdgeLists(); + + // Initial assertions about OSM node 337954642 in the test fixture + // It is an emergency exit, and it is tracked as an intersection + assertTrue(sl.osm.nodes.get(3377954642L).hasTag("entrance", "emergency")); + assertTrue(osm.intersectionNodes.contains(3377954642L)); + // It is a node on platforms 3 and 4 + assertEquals(3377954642L, sl.osm.ways.get(899157819L).nodes[10]); + assertEquals(3377954642L, sl.osm.ways.get(1043558969L).nodes[7]); + // The entry in vertexIndexForOsmNode map (one of multiple vertices) should be flagged as impassable. + assertTrue(sl.vertexStore.getFlag(sl.vertexIndexForOsmNode.get(3377954642L), VertexFlag.IMPASSABLE)); + + // Check that Platforms 1 and 2 allow pedestrians and are connected to the rest of the network + int p12v = sl.vertexIndexForOsmNode.get(5303110250L); + assertTrue(sl.flagsAroundVertex(p12v, EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN, true)); + assertEquals(68, connectedVertices(sl, p12v)); + + // Check that Platforms 3 and 4 have been pruned (ALLOWS_PEDESTRIAN cleared, so no connected vertices) + int p34v = sl.vertexIndexForOsmNode.get(9604186664L); + assertTrue(sl.flagsAroundVertex(p34v, EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN, false)); + assertEquals(0, connectedVertices(sl, p34v)); + + // Check that the stairway on the other side of the emergency exit allows pedestrians and is connected to the + // rest of the network. Actually it contains a barrier=gate node, but it's not tagged as being locked or private + // so remains traversible. + int stairsv = sl.vertexIndexForOsmNode.get(5744239458L); + assertTrue(sl.flagsAroundVertex(stairsv, EdgeStore.EdgeFlag.ALLOWS_PEDESTRIAN, true)); + assertEquals(68, connectedVertices(sl, stairsv)); + } + + private int connectedVertices(StreetLayer sl, int vertexId) { + StreetRouter r = new StreetRouter(sl); + r.setOrigin(vertexId); + r.route(); + return r.getReachedVertices().size(); + } + } diff --git a/src/test/resources/com/conveyal/r5/streets/delft-station.pbf b/src/test/resources/com/conveyal/r5/streets/delft-station.pbf new file mode 100644 index 0000000000000000000000000000000000000000..44b672cd63e1c33af31845f2f84c18842d5ecc09 GIT binary patch literal 17046 zcmV(-K-|9o000gO2~Sf^NM&JUWpWsR0T6W>e0ZGWlHzFC^>U|zz=2(-_b5m-Teze)%b>#Z8QC26`qYdWKz$hR*Ju zxrqe|26`5H777}MMuwIq2IdOI1)15Yx(4QY`2|2-B}J*JB|uer`9lZ=>Zz+g8_J)&AbO(RLAx>erImkyO-T%L7LbQd+#POrbv2y zsV{l&`|^CKtCA=fz?9blNEeab1*D1eCS5>4L_tKFfPjdAq7*?yK!o4S-CaORUh@0C z-{=3IWaiGDd(NCQr_GsJ63$1HBRIjX?JulDl_P(anQ`n#hNgShne9Eu2@V<^IQXOC z&z!*QsOf&gy)uqGWMs}!#)gMw9Q*NyJC2&}F*N2xe>X9f?Jzer+`r#wF9(fHI0g;y zc!z%8YyO?=5GOuj_=A!0f!${2=!+RA*l%KD&I1^l9XV)t$ed#j8~$W=L?+wEab}0n z$AbvWjFY@?Vzx(SYOeR8iLtRv#tFVSXeu)?m2u+F%?-_sOpMKr9FiFsb8yQ>PIS~v zw%g2TpX`XC`9Y4^bJPqIP*t#nz8o+%JcL$0@y@zBRV}2CPeu$$F z8U8FY#ShFQg?Z$-eKH=?9C60-;CNQ*fQiXI4sP4Xu}2OWn(sF;J++RDl_DydyX0%+J^;x#L&#l z@PLe?&CF#-%s3Tulf$M)-x}|c$-d)g+2J3MP)Q^zPGDrb_mGjXj02lF6;l}&lCimw z;UTnWQxh{YG!;jh%Jy^gk4F2Dcv*9#6(d7aB;e0X_I!>M_npjmkExOIcL>46c(17p zu_{0o^3JBshMf3gBiYd(WqR+J8tz3rO3V$>IuFTq%Z-AgObrho;n;(Q#`{bSjSgY! zF#XZcbRRe8kA}v2AIdPU@j(;WcSj6OvDyqxO^uMASi~WcCR#teM`n!7Rs5d$3Nt-3 z6TSUNpFeXVBV)wnVc9+-EH}o?2L4`c^S zWe0f6Q^$fp^Xq+O^zGj7j+kKk;jl(`?=?A!tb^ety$M;psnI^9Z6OlO-tWwRJ|e@^ ze>CPa-jp5MZ@yMf;Wm2wE7Cmj9p-?k;Xc_Wj(gYSsJZ!3WCO+=*uqH+ut@ZNK=luY z5IZcgZc{^Zv=T8E(C))X7ie`zP>9xHPK^0RANHBaev}>H#BZ3$j1j{~u!e?6Ls#=L;peaJ)PQO8(j{-X&p74{>eeIFSjzB$!5vGpOIWP0x+ zETpf5V4@P@T4dt5Mv<1}{Id1dD&70S4+`0~d!_8YbYV+0&FL3nc?JsWI%8^^v zZ^O-Y4s7E9YIkwWuFu}U@kw*nK0{u1@0uLqS0tB%%C18=oPDy}VAnhN<;SCcH#v-q z!Gt3XUVej9F)%~{)zom0>Cpq6Nb!Qyj07rq!}RF)M~#k}DP{t1a$;VIN|kcj3Kf4~ zYWST@cIb%7AtX(#3-A^P-sZqN9Q%&Zw`Qg$C~{~cPeitAC3((7VC{%Im zCo)r0Y_{XQx5j&KkN4gg@4Y+Tdyf--Vshk&j9(7NcjW?QK3(_eQS9@d85$l|@XCH} zh>aGZ3qO|~L7@^EIw#;wQE!v7vsu~MqU>ztL|+=2o0%K(TbYcLd?k~a9Xx7;HYYTz z87IQNG42Zjw0RmEng7hOhDQ#|_8ye|fI=F!66F2V9<=jus(X+lni%hvdmO*<7#=a) zi*Y0fe(!gB;}MAC-*@lBF-?TR3ZijTzK5tPHS7O zp&WwQ|D)Mqlu^(Jkx|IF%?HiRkC^S)u;GYY@9X!P9Nu7jh<}yIu&JW0Y{Ncm=j+TA zW>0-5Gu?yYG-g+XSN5{`K~s~X2M%)Tli&U+U!?{f`=@eD38_2Nr~*T9$mB;pOrZ?L z7)8B($aGPzf;Qy?cz-Z9!3vklWrs4M5F^bQw7L8`=A1KtA1@foCc)1zo@D5Cv{G8UB5U_;<2Gnu(Lr>V?fc3^ASp-j!R#)sv) zQjRn7=*EfU8=yjEDrP7O9zYupvP%?O4fi0jG{0+1q)dAa4;{vAYfg=mXeMk>IPmWt zPY*;#Q3ips4Ws==XhAARu_qlc^mf8^}fHW_xzo!5=GYCS{>&OvUjz_`&x5@j&;?*4Ky?Tmq*FC*8ePaA#Avtqis(uv;eG56~K_Jx-O$T|IZAoRSMw3wbXLfbU92KE5w|& zJJXDhgppD;Wz9cTh1SpE)TInZ>FcClkCrr5ba6cYfusm}Tq^iH`(l@*s?|n4p^srC zOxz!(Qpj)+R;2mN!8~bkgWY%Mp-y^%jRtgT8&E4eTz)TG#5!r=#YFl3km+0g0O)RI zgH`5>7E){V84<-u=Pza1ylC2D4pU_vbBI#sYRkIBBAB4V>Fv-OAlbo38a|Fu`i!3& z$0D*4mZ?&Otc30Scl)t$j)M;`c+wSCF=R_Z0SUilYxP-a5=hFu50^%gTcLF8K1{sj zXXy6BRwOh=8rLO;Maee+!%|EDNktX;D+Ny>fpd%qNw{0UN)a)r3#IIJafKK|13#**tP@Swr84u+N3pU8!3)ZIO3@vv7A< z5ZU5Sx7Qal5gmi?b=IZ5Q&Bhej`P2HBVc2it0`z}0NaNvr1t>KEC5*TIm5BYgcvE- zFl5$pB0Z6m!N$q-pSkpNX|}so$gLo0n2q*l)&mP2^o z3i!DQ6cAQFKv+{Q=)a#x`M3;|(7LoqZqfnu10;Ty@>aR|Uc{S^d?s-@uXnY_jFv1rfpz&P>1_)4ju|q=AF0)91^1kx)4|TJ%xB*|`u> z{crY{r)+}vuO^G2DA=XWTm*IGlv!hs0X2ak%Jq62`x02 zt?|5asTp3MAETUMRDByAxDCR4JvM5vFh?J@H(uUpUw!Fg^=ZbkGNL7XuDt7!8>$5v z+uOsECab9TEf_l;NscxA+CX;|=C;k}js%}GQ`16%i&4n3fXz0$=^y;pa@ z;MT5V_=C0Om=h0?9TqBWxl`pc!{T##(>{FIua9~mPtuElTYzN`fFb9*ssVJW4>^s- z?VI|R!?4iv%Wc!2{uM%>j}H|1Hpjb1a;d!%qQ zrd-k*+`D)R=<4`gTp}wJ2vh(B0vb63i3>$iEW~|4BCjQ}Xc(G857cKM$+8?P#OUaY z0-lsZk@#OcsCXsi_!K!&Cmy(iX+R_u)W{2D`4(VF0l7UZ-Y`Q7ws{be$DtsQgYbm$ zh@39Z2~^BV(mV+zAW+hp0HBnNkns7hjMA}41!Yn(6Xr(FAI61~I0In(F+SPYF#V=Qx~6(D1*fwf{yTXXX>BoB{lbX&O#JP^fzLIeBeR@K?ZiG0Z-N`mkCQ zvp+;!E86N(b8cH~iv3AY5mLIvDgM+o9yIxM_#X+PdbpM&YEV;d8!n3XNbmqex%+W@ z5LxTL@(ik#^-&uir#F9P?Vj|nYn^wfb_St($Cmv>5HKMEJ>pS=JN$<>hJO&AJb)K! z-Eq%D(V6_jVEjk^D@JT%2;86<RIJl;%yEB5eG;tk@d^e+b!S8Z z@HOw&4EA*aM?=D@wR&#VzM$Z% zH`m!T;?h=v@JnfQ1B5Sb`K~wrZXbG#>I;1M{Udw3Wznf)@5i7T&Jf2E#@@F^Rr$2N zo$m~6a5K@fzT2gDzP|Nag2+MbG4w{@Aw=_f{+Z7@v(9|>q^?UZE%4M^55rEq<@)H# z+S5rDI+&*vS3$G?sc%yYW9BEv<^99y-iYAR%~visFD3I zJFM!OUTjaS1t8EON0t*rYl_njg0M*JQqMZs@(v_W0+mzO_5(rOj4b`W0o6Mmr;ZMW z2Y|Ep{ef-#)1R)DJlOTnA(-t!_1vA4ymZi9DX80-;;?`qE{A4)9^9m1<&pQ5TyP;Y zF;wYG@%=M#&pspSD+`udTRqr7^y$FiYk#_ma&OwK5e(m zHRZtk~pc))K@+B(zhVxjPB#%+QyQp-K>GMUSb(gm7J-QMZ?a&eXhk&%I7` z7geI~30v>=m3?KCR=ex!COf-j{yDzXlk~vtd69wJ3F2%;(N@BG(lo|*0@^(;^H$0e=vBX@-IX};WNqIr(=7%Jw{eOKs7bZ z>WfC_?p-(Ivvl1?cBQ*NTQi7c@dVWl8|SsPJ!7P2v>z35LZfvkLM1S>ORo)?j=#kj zl_t53Iaw9FavW76Dr0bCM@!_|u0WFLJX_CXH%D%%c8Oiu=HjMPbH4je)u_e@yix-w zBI;(zYUi8%n-O|qP|?@nOs-2WNt8F`z3%Nb{2DQsm;7_{%_LT_)arnCOAy@T{%T21 z$))i0EwjIUH5OuNJ+b za=Y4Ne&%C&A9_9Qnt#o6a8pB9)w=|7y7J1F5LD|Pwy!7pTKYG;x7EJjfhytgs8h?e z^4eY^#lcaNNX88z?-TMj+<$&}l3Aa_BTzYx!K&)diJRl91e4K;c+@1I8h-tr20=93 zvZO~_+EuYlV%w}=G;+J$e}0{)->;Xmb4W$%SugvvRc)$TJL%zcKyO$Rfp|F$sG zhJLO5T5$LcB`4OGx-$H3b24a$Rdu5pUM;}_;#l2Tt$-F!K=fr_C3&!iv;uzQVlVa? z=lJ$*Cr~Xr=laT3R8auVOy-JHGPLeAR;oEaen2_5d%qWiYKBh&HHlt~)oGY8T)gdc zYsMC$$UXN>Pm3^wO$6JHFgI+m-PYP}_USu85z|Vly&D`k{LnH6^q^YrJwg#NrFS+E zbrrQ#J*q)=33CyFtU%x#&DP(y198P3i2K0&9wehM4-dBDTKRL*YL61MMF~(#pa*f~ zk`EJZ>rVRJ4Zu*N$sZ8T4^}(o`-3Tb=w4DQbWQ&{C^KutSW7k&(_Uf_-)p&dWFVG# zifUw1^5+QZYGCqfWvD8#>>R8O{HjN^+aix^C!F(?q!v`FRi27p)pE98eBU$lj~M1r zOX>#Xl?fpfQG*)iL>FdgC}I=gQs%X$D9BCgBC3~y%G55!70>nZ>0f>(@Cj(f3cC7i z1m_UlL+H_k8rmh0-y}Fd{5b#{Cpk^BiCw5!HTAvj z%-cJO>aNILm%&IFB0K!B07OW-3~RW!Ed$($3)>wzJr4%>z^k9-N2A>%JCx zLDLe&->8z%d1p~Qcm0kacB~V$mX`~oi_3*X3~mS_?takf`z`U}F3Xtt3V!N3AL+Lq zLmR=c-R(`p5NdD-KZ&?LYhX2yzT|Go330qzmu3R0jgYv0vP-i9RpqlOz~Ze&RF%)Z z%PA_!^`m+yBB8byy%Bf_(RvVcTh}Szwr+Akic0R)G!38fG>yb2TS;4Q0`Je6^&lp? zVDacZ3$b_igEckLKF~8xHTRi43?6eu*#;4W9=MC2fl&^N%4$kfA6Qhc$H)N^~-@WB)3b$gH&`Pr~Qbv^T(NR0|S3 z)f6HrMBP?zXO1AU93AHM2h!Hv4GMAPki?Kpo!rsOO?;q zno3lO_VO52!YA*9_({v?yfB+?=H{_(rmC=CJwB&HJ=9hu%vQzcT#8;$aiCsiL&Tht zzIOG7I2C)FR5HNl8kgtnxy-$xdG4@62xrGEbnLx_#6R6K9A6)*?t0B*)KZI3e#N4~ zeXP(Y-E|TLFU1KT^fvXWM#Y^$W*3L#m0_KsLs)p+)*f@etxXWFK{eo3!4-D&+31{` zv5s?w>+Gl@REhLlSbTYYYu{Nxn3cHf+zIjN;aX78bw<6r^^7|5Lp z$G-36f}kfi=8qP))h~ zU`b_H?ShO4ciB8g?UqU_RV)Q8jjnim)r8JS;2WO=oJX0!{2DSQDMbYv%;Ulua!?+6 z9i9BN98e`PgRImp)?ZLPj^bS+q7u{-s1l6ospEHr#BfW>%R{IV0rq$K_~dJMR~X_W z?-NyV9ir0-VH)+N{)-9XcuDhICsZwyJG6}jD>O6pN ztd(AH=>zE4@U46nE3$TcYmO5>w>`}2pv|*?`hQT|dTT*&RCV(ML8_DjPt`$(7Gv(itWXFYFzKqhRBi`vNWHD zG!S*r&+cEocK_<@$tQ`v`X(LX*u4yBjq34B(ISEvta?AfmvRbuEzThoF+TDLc>@t} z&x#7m2zf3oDLO`8JVv6HKo4Tj+nu~MR0aaCdSIyHGs}r#YYk_cx2Le6Un_DceS_$9 zbO%=+rVCGaT;aUi-|%UFqd&Y2?^=)0L(%2SBfei4>I#N3UVZ9MU%mR#nY)wta42V! z(l`Q*1)R9c_@hch-UUjKNdAf6CJjz}K_mDne~h=2KmRAqV>q5&%%@_yiGl_OqY?zy z3gQIBI5pxqjwT~aUc@9Wr$F;uDLI|Yx0|KA&=pnXbIKwVDDsEpO<`0%j>`m+{=?B- zQ|EtfVq$_x{$qy6+Lov5Cf*;0UxPiUj^iXEjX0LG3f;&H8bd?;#J8WW%k)?~Rg`tZ z2KB=Rv%gQ4$W&oYm*xMeAYK(lHBNMrbxbuSR8pUoaOY6{5A#mKOuL}t{%B!o$_u{& z-dFy`Xwvv11ub@YJ%4_k~P80cM7w(AW2%ay!kxOO6g8pOn}__6h=7V zUfI^U%8bMP)OiU;s%(0-s_9kv7y?A(u?9TQR}7&P-y{kJ=?0xa6O1U0ohOMqO7ojA zd%42Lbvw-?C!09xUrk*Qac$lN)&%hhYnpB)*nt(~Z!7Hj;mJXTLeavaY|mj?oJEg_ z#vZwEKCRgilp6E<&dvWH6vg`G+94)c$waB+#o{vHta1`n`{X`ohbr2rqjo59rl45( zhk`M`P%!OVz(nMlRL0|(0}ncBQYE_Fwz5#m8}#%UuKNoSpyDHbBc?5bAXe9~kjR4< z!O>!P2`)q<3l=KBq2o?AybIbv2Aq@`XsHpT{$hfX58)((RK6%jNQ%HeO_IvjnZ-OA zWg)SEoP3o)wm^JdArym{!o+7xew>!#8=^t+l_HleS-NBrSqcMOAusZ;#Sm@r3V0rv zP)>^m+`tK-?E67qh`y^6np(;;nYt!IaR#o_;^n~uGxsNWoLC4&&TwP$e^b=eCKgC3 zRzIoy{pbIHuYk`d|8eoxuBN`8{IcUE)JP8yONk|f7NI##8<&H7Co_P>@Ek`t*}_`G z*0WzRW=Il+us}gwOOqn@7|PeAz$``|6?5n>)+ZKYt;scxrAjKhlNY5Jp;}-YltalU zldEN_=ocm+~`Cr-?|KAm~G%chBxhf{gK?0kH1sp>Q_!NJC z?sF+wLOwU9PfROv|H8KyJURwON|P;}4k^~egJNO|3j|?sPnuu(w1;%bhNC~H>t%I- z+9es95hl0D)Ai_(Ir>4vA2MG_*_5T@v!nlgr$rG|-G;~7K4)O=9m?Y&EZP+E&hcz$ zFA0?P)2&pkFosU2<6xbJ-{Na9e}$vaLwj(c>$-%$4QGPWHXzy(KHL~d1qh-jTiW-{ z&9B}EE>NolwgZ9jB`^Zom?RYgZ>F2t)=Y1Hb%?HDTtFkNAiwqkk3l}X41A#@l?gM! zy9OnqB()ro4_QjtF%|4Xc$&T}fDaT~P}VJ`^Q`2+b(fdP&}5mkS_F&^`)-mqji)HU>QAlxy0q z?;0T!X#D}5W?-S>qG2WQd!^`=#BD>_FlI|}4pk+Q6t32A+?)bapbJcb!*d7M$1xZ4 zL1G*%hNEQkDe%+_R3?JVwPT;O2*U&(WH49rh1b0F6CmZy0r7}RC~cuu^FavJPqmW~ z@cfq}g8WaLcSfkB(oQQ}d_gDY_Imw$VH=v)m5E~ByRsu#r7fL$NcMw5PyxeWJ{bY5 z$!^e0`cVA>du=DygRJKo>3%Yp@?g{HX2^K3<)UONkSwJKR72P>Hbh_zBG~&(HfU!w zg0~a`jaHaV?+8@iℜ2#+E!uB%o#Bf84^iLBOG2MDfe-P!57F1MSy`K|+fV_80| zk>tZDn6u&*cUh8G3cNExGv%mymQL{j)${I163si^L67?5Z-Z9%S%Oroq?gxU0AWi3j;SKRc%0~0_ zCVQQfjkkjkMW12d$%bk^GR#&P{^Y#c`L%cF4N-+u2b*&i4her8)<~cXLO~)`pi?pD zqPmypsqpHOyltmcx8gzh1f&DRX;??jvFd`XsBJ=b*Wwy(#^m@O}J9JmY{Vceu zp4JBLf*bQv=eB?_Xh9FWG{9Ifp&wS#SuZwN@`#xjF8l*I=PS)WExdMISK9l@L1|A?V}Pz*U1E-i_m{jg|LSj z@`mSyA@5YNBYW<}0@v?DcGZw!3;pFmN9Re4Rj#eI2vH z^G`J|M!s4z@0>I-23}Q9z6^32L30*tECLxHSOMb|8Ne0Z1drh((t$2fYlG+EIkEuu zf&-!4y$BFu1&g4uBgg}Rpaz~RgBGAA2-NIpUwczzH?L4?K?Z6(Snj1(L|uf|r1MIoV+-_PoY`Tu{n;QA?Af6uUc4zx>*S&?aBq>_>g(z?8{4URfwz7Tcxwd$ zU*C$ro!A`pOb`YBwnIy0$vfv);cqXfD8CJ>9n2h`>e{RyRhDde_J4;dUi@;kII+IFRdP`+k>-OHlg=O3N&x2?0^&GtH4_ukXw+3P1Qb(Ef>k&5K z&PNTV6)*O$b)b%A!?PdUe9z%HNE-mx*7@}UN0HBKp%&nRFn4*yg7gG3*&ygMC(}|k zA`i4!f~V$|tuXJvqkmr6U|j?vyg)TsY6-6WRJ^fBFw9&hErpNYKCj=YnkTHFJit?7 zsx)oUfGfCpkq)>7xlIoAfH~lkHK)NdP>rnk zCi#AYG~sdKsw{C$IC(XKY7e3;zE0dU77Z`Y2`d7ra~$T`EcDdzoa3pMt35_ruCUPR zfW<|iBOF>^A&;qE{kZGz@eZI{&rLM}lu{2>Dd7HVe<+I|Fw&Maj(XhZ>8X=8P$)Yn^ zF9yTZ3+lNC+SQWMHIG&9YKF~s5Dm|9e?3Q(t`Z|2U0%ICZSlbB2OmG=oVZ5K6b*Zk zjefc26ZMvr4;FgO>D4$osBv5)WMP@;?8>k?&s2SMytt0}_vSRJmTRAs_^}O?tp%{T z0wRxrOM&3tUp?0z7xik_!jZSa7JfVel0XbNz9?`0CB4=^?|AyxQb8fGC;Npq+jGD( zbqC6h&J!i6^y^+Vbeo&Px$MeYdio!Qdwa>ti!7;1eLLn>KS)0YD-DvhUFc+$8-gTB zw$w-6?>Lp_PiHJ_wFW`AK)^*%G74&2LC}GaFYO&D?_B1dFMV=*DxD(npu7z{)rJ;F z(SP)igfi(|;Rgj|$b8Rb-n!-6yrJ(4B}_PTMkTodgzTvChbLbxyaC4cjmbt=hW_m? zRlEM=FK0CG!u(YY!fN`;_G9l1a*srVTA<)$mJAx zwx(CF{WiGH*{FK1elP&*_g4Nfdxs0l!mU37G*BV**9RE=ns|^G4E9i9)Wcq z*rAj^S=(XN1v=@A;Re#1j;3Rqpv`>uP%>fnV>d9$lnNZF{5jd@K-jkCL};A@&WNM5 zt8~vWRWc7cimrjRpyRy|?#ix5Y7e+4RF~f6Up$5W+wESvyvlRMZI$>p?{2%n-Tbmt zGfw!Bspb3x30x|9g~_9w8MI<}n;nBi(h8Ml3!|h@)RW<;s=s=J-fjKb1qF*5qyu8R zCC9<{4bpRJyX^H6^#aufB)1kPs0~Vz^=QCw>RJo+S8m+8^Yz+!uJe{x=`Ok(0A{Htsh%aHVfBwO;&kJ(wf5rcujWV#RVvvNf9lU9Yu(_9JA#K#;N!Qf z20_CqkQN19jqm(5bXmOqkXFSXoOidzfE4Z2muxrX_<={oz+Yu(;V-x8Uw#SZvNUa( zoM5tJ2-b7)^fi^{IOw|S_S|E`;9d!Aoau6>;&Q?e{h#nA@V&K;6eXeY9ET!;7qY+Ce8FHXH#i#@FqrER&S zVc|2Cp*m3Xl7lE;Ct~gs+H+Ue>lY=x(jdv@c~?%re75+T%dcF12!r3a9tFz81Bo!} zO}F_Ew>GFA$A`E^^n(#Xsf40n$knsjDAN=Q6!$mu>)0NndfYN+*>-=ZM&0{!Rph*LsA_@B@ID+^diS0@NuVXkyI5@gV)xG|v(b(cO%*)U$Z`!(jh_5B-0J+O}MVtT|M zeLlzF+kbxh*Ds#UISKp)@yqjKz=^kF*%4ua#7lRd#@u&I-uSKy0rxPcBnsF=O=|n` zhquryH_(SJ@kd{(8GQTqZ~ywoNcDYM&(AOa^0zOX)Muj{to|ICiP_#A^MHYmVEDfa zR+i<1XPxi_6JbT&)(X~s91WgoU1U9l-SJ=)+Vlcjx`Ql(sc`N6mG{+BRynGAGwxI+ zh?#Ru^|qjwy12?d8`Qsl;ZuK6gSh^+^J!Eg-MG1t&eLn8GtGL>0E;-_%kl~qe?meucfMP~~)V6iA>b;9fIFnCeUoSTy9m1A=s zY_rfC+YmkXjFz`%cM`~37`7swwg09}-EQxgN`_j$BM6YhBC|U66!_Q!yN?^=z*#cq zm(xZMx3;^2>+j~Z0q?hayg~e{$F+Sm2S19}kL3n5o~Z<|Eh`4qGPt{Bt41eX3cAE4 zwC4|wB``Mzw7+m127MC*qYw9KTE{^fD|ks@Nu49l!0*5IfRW?~X-gH6h0qQB&lvrg zFaFJE#{i&qRD+wqV3r>&-pQTqzGyoX>|goC%iciQ^BM2?j`sq`dqMxTd#87=DyI6= z*Y84kGx4fuF95hhhuUEP`v7MFxJgjYf&H$sL+8(r!1!afLg}pf__xkT>F9KVRc|3F ze}w;iz96}Ift6p3KNXX|H46>-r{6>Uzfi^(@T&zGSrOoV&CV2TQ#$_*rp6W2i_&X( z^E|1yl#QIO@@0mCZMO+zBJscM@tf^eXD}nA&_Dp8a9(B=sW`CbA_~)UOe}4JHA9ckLLen(x)zKwO zUi=E`Yp{!?)m8bPj85|7>>YY7$C;I3z~JSP^Vfr3jrQx*h!F?cP>f005?GQWA+?Bt3<&fmdQLM6jh!}BMu7%%B3QnPC)8{r9!&Y zc9bvZsCFuVy6af0c6&ddE}$4GVp6Cp(1w~CP!&(0GTji?vB!QNxb^P?x0w}OG!wY( z?*q5{ec<-9f(vH?cbFBNoeA9W_kkb(ec(>N5B$XMS?WuyGY>9Y$ivS?2tvqXe8!^) zrW+p98RT|FM-!o*{N1Qt!3b3ip}PDk>hyIPFxD7V8?Ddv_lV;a*5mgw$<^TXHG5u) zS&FE6{yxL;{yoesXFDP<_ycv4VrX4b0>vos3Z%!ApV~1x7caDL(Ytf!620_w46Kbbl0+$ah8z?7_S*G`6Z|s0u<0wW4Z1wcJONdSsa$-`+ypbr7CjfK3gM?rc!A= zPb6`ihymwuy*g#(IsY?dQr*>T=Vs6Qj;{YN&Qu_F&M+L1@a3(-k zL2eBjHWk(d;Qfu3)>OqI8eX|^ujpNmctEY3gVYhh11r^`is0B8;MTFF_O%F3fW#xf z@)02UXvhULXC|YtDhy2g9Tt0x2cOMiXV`1F&eK6{IzFR)mq^`hY;#d(GEim(yZNjD z;;bMMaEh#em6n9yDQ@^qGR50G8d|$Rm)Zie0D_~W9_`U5866kff)^Cp-oz#pV>lr; zMfq|NcPqvh#Lcw+L>}cgZXB<$NpcFmJvMMw!EUkhAB>H)kh*WO$vnEs-i2}w@dPSR z!Q*GMP&E)+<0)LNLq2vv71>xqVIP`><~6L#NRYcN79&(z;Q$l1iPjeq)s_95^}C(D z;Ov%%7}Q-zE#NUeK^oH1dgVo^xeKb+D)P4KxGF?uxZ;A6%nJ5GRbUa;5Q+fQ`YeE> z1tq-`=U~t1xZiv@9xYa|g-Pz0fI5YpSC^8&55zntx0CFB86EpTubqk+x3kunn;CEMZ&wFdke@>B6;oyi_cr#;R$Z%-jp}6qB!E z@A5#*YHGd;Jx`ZeyM6;y-OO9nG&fzxo(`$Mf<*W|wLl(3jJ03X|ArT#_VggKj_pgH z4*pk#dyEB+pmhdDmQ8@iINzKFoSO+ec2@8i^63}Z3$2AEKwU(QCvhm=iuEAmcA*qb zb&oxe^EFMQTi8oHCO0mB6sM6PtbqDQZza2Z?y)M11=7l$D= zt_G9=?j;WDCxbljfq0f|Xc>EppMs>2eUP7iAh+w3Kt)^6T$+hcoh=tPP zo*S2Fot|3{v=p;)bV#U%#CGodbv!dWB=X7c@enuNBov594sljvaCs35a`5$b;f*Qm~7)i1Ig zww84hJJZDZeYOne%_Eo{QQkef5<5j(UzRhWTGo6{#6F}W9%^zcR~ zRTUfI479q>U^I@Jz~Zp2nINP+vuwKS*j^s#d5bntot4J@qWY$3>%sCfQAYV`;(LMZ zNWMWfXpVveko6grx z!YyAS3ctJt`9!?#98ScW3MLZqRcs88Ls9b)IQ8iU1gVKcm+S0@CU3E8-l^yLG>=+& z3Lwx}9u#E|*qUfP#QCg@$IZ$(GaY;h>zGmWM7jM+_^2E{n0KGnxs^YluaMmm_V{lB zI`Lb8oc}Y>cDDFQ$>7YX#5c-RLS?!St!C5e?4I(rAmG!i08kEi;RoEw^k#JK#3#S3 zmlXX>u=0<(8`umUR=}Tvkm(5VF(?3hP!9OXk59Fq)oeD8Fp79%uO-4d{HA|9jdY|&p9;m~@7 zkH~WSnj!o*dtvO^NYUopqfAilW6|#mMDoX^&OIAgvRgdSbPnMT{f8G6xu_@W-I469qY9e6bOG zW-1;&_Bl*FE*q#`aT^WalIHAZB@S;X%n|FZWaDN)6-fPDj-yojQGMTdJXp@g^N^DR zDJ6}Wb|PQSCQO54W;|IY@^CX9C1!{OZ?Z`|xWd4Z9C6UqKoU>OYxH4svK*{-DyBBj zaeHDPxojXe_{|ZA&LZOWvyaed9(?BcPV1O(^TKx!k%w*vVhZ#5T#Tb z6^FRq>}Z@&cf`Cho?*6K^RR-yezck)V? z*%%&2fTf0o#-hywa@M>dv(ngs6oh>BM#2PgciuqCip$wfVUzGhJOC~DT3%-uU(uh$m-Tn?dknt7_vbI=SFru*_O=k`?(65_-2ES$`S?C9 zA6d?3msBA@l(y`{Y0E+0m$IFs-5H&m>G3b=MMPc@tlWyiz^%vE9N^?I;I9Y+Go*gY z*((|0PH<9|gWYGE?V>FxDB+YFRBS<8&}2Y5dUnczb2;l6cfv!#BDMA*ILN0TrOmD< z8J*tVb1#e=p)(JsG{Qf?w-rX{=;wzrl~TJW%1mW9d&24-0u{^CtGV}4_{I10{QzZH z8ZZxt7mFRi%-0?U`h2LT!0BV|+$|87c4~x9#0|LM7GX){;YRm$)|n@uRt%^Y4lj`3 zSIC3BlTAsTZ_TAPTtdazas}8X)-t*y7?YeoPnXzsbvGhcc06ZS02b8fIpb-5ZcfV46>3h%|-&wP-LI5jn^UCyS@02ek5ZW)^~ z1r9H0cvhJf25gWR{Tq3uj4jp&F#drR59V_Wso^4sW@ z-9UFEoBccjjxZc&2`~mHJ#ZC`DN|39xNV5FY+jDQol`x?3HOgot@Ve3L0qm(YYoSvnk_||y@_V|IQ?zOzh{H$}&Jpcq z#d4d9#U#l|y~x`0NUZ$Ch0u_F$~N#^;yrVE(!!KsOl^ee@Ov?fF{UbF^dyf-Q#_xv zWOg>$6pXU-3eFu}WEf_L&UBPr!FE+XN0@hdfiz}n(zca-^aulTo8CLH9eLf#&kgX^JA_MM6j%b~ z`Ap390u%*;lKWp7$BG%91uOp7uo7m&iW|p@osN|}8&>SE(Mp{SE9Td*GG@byo*io= z>ytKGhD=>og0DOIWA0oD%A(}zW~3cckFHBu|Gwr*`L;_+_#bY7G^-;EozP>#A1hDj zudzcpJ@s(LzlVT|zlWH9H(|g!;gnZEs_DML4*%awAmaa^A_@PM0>HL@w-`^N%(Ue{ zVdlgqqs^};>iD>=Z)ETBaO~cRt&GpFe~}-Zp7I#+(GwVJ*$y6KdXy!lcGC`I8(6Qr z!lvi<2E0Wjk6yB6bc$;pZ&v>7T#65eIN*8n2+E&e8GQFJD^i&f$>+CG;R38%LzgGv zBPbG4OruTrlueP8Ho4@XT-)_w`b*;%$ewK&PM&SO&4;Jyf3u_NXjgQ7#qGlJ-vZ&9&^HuZ1KFBjEqYK?KXOl zn+4E-E$MX+C8yjH@ZoVv05{N1I#)NavoI`FdI8n=`mE3VT;u&RIiX5`w^acLB^+EMo;l6Hfiw1e4OYO1 z_$J+-VY!XZaA#?87h_KO5oywIX)_?LWfNSxJ7;uGRvIwXIX`AITCet! zGyUOY25-)P`AU9(nr6-#P5AF7kr6Mf#U>4-u^A&xJq}KLLOyq;0%o{US;-FYaLCW4 z_=kT^Q+|x;dKQnIFDE>1(yiPR9&%zwm|;ExRsM)%;oO@_h4(&8rI0B&w)EOl>VVP( z5*}%KmMh>B@wpnvaF6rAD7QrCa><&_e6o7q^EN^VEIBrT5H)OZ@qGrOWjg&91l^a} z6TfNVISF`T5d)r_wDs9!?SdMyu0HGfpO%0a@e3Q7LIBLb$a2>IH^K${Cb*^SDIN|| zYDg=KXN3yjeriObi^LYH(Re@bSgA5Yk<$JDflSMfF zdKzsx(GKot;_HC=P?stI(44pUX@A+Ujt%SW@}wr+W8?4wdG7_&{N5hya{>1R{|5m{ FChXXNm)HOR literal 0 HcmV?d00001 From d1cc75f12dcf609e14304bffa18e742190975f03 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 28 Dec 2022 16:40:55 +0800 Subject: [PATCH 24/46] handle single mode exceptions in isImpassable --- .../com/conveyal/r5/streets/StreetLayer.java | 36 ++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/r5/streets/StreetLayer.java b/src/main/java/com/conveyal/r5/streets/StreetLayer.java index 6864aadca..0d2dbbfe7 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetLayer.java +++ b/src/main/java/com/conveyal/r5/streets/StreetLayer.java @@ -20,7 +20,6 @@ import com.conveyal.r5.point_to_point.builder.SpeedConfig; import com.conveyal.r5.profile.StreetMode; import com.conveyal.r5.streets.EdgeStore.Edge; -import com.conveyal.r5.streets.VertexStore.VertexFlag; import com.conveyal.r5.transit.TransitLayer; import com.conveyal.r5.transit.TransportNetwork; import com.conveyal.r5.util.P2; @@ -1072,23 +1071,44 @@ private static short speedToShort(Float speed) { /** * Return whether this node can be traversed. This is particularly important for nodes that connect one part of the - * network to another. For example, an emergency exit connecting station platforms to an outside path. - * In the event of poor connectivity in the input OSM data, we want such platforms to be identified - * as disconnected so they will be removed, rather than staying connected via a locked door. + * network to another. For example, an emergency exit connecting station platforms to an outside path. In the event + * of poor connectivity in the input OSM data, we want such platforms to be identified as disconnected so they will + * be removed, rather than staying connected via a locked door. * * At first it seems like we'd want to detect nodes with barrier=* and treat them as impassable. However, looking at * https://wiki.openstreetmap.org/wiki/Key:barrier#Values we see that most of the values represent things that are * easily passable, and even barrier=gate is implicitly openable unless tagged with access=no|private. * - * This code is hit millions of times so we want to bypass it as much as possible. - * If tags are present, memoize the values rather than repeatedly extracting them with hasTag(). + * Even when access=no|private, there are frequently exceptions for single modes. We don't yet handle single-mode + * barriers, so we want to default to the preexisting behavior of allowing passage (and relying only on edge + * traversal permissions) whenever we see exception tags that aren't clearly blocking access. + * See https://taginfo.openstreetmap.org/keys/foot#values. */ private static boolean isImpassable (Node node) { + // This code is hit millions of times so we want to bypass it as much as possible. if (node.hasNoTags()) { return false; } - String access = node.getTag("access"); - return node.hasTag("entrance", "emergency") || "no".equals(access) || "private".equals(access); + // Always disallow passing through emergency exits (the original source of our island pruning problem). + if (node.hasTag("entrance", "emergency")) { + return true; + } + if (isNoOrPrivate(node.getTag("access"))) { + // This node is explicitly decared inaccessible or private, but there might be exceptions. + // Err on the side of using the existing code path by returning false. + // Consider the node impassable only when all mode-specific exception tags are missing or clearly negative. + return isNullNoOrPrivate(node.getTag("foot")) && isNullNoOrPrivate(node.getTag("bicycle")); + } + // As a default, err on the side of returning false, which will maintain the preexisting code path. + return false; + } + + private static boolean isNoOrPrivate (String value) { + return "no".equals(value) || "private".equals(value); + } + + private static boolean isNullNoOrPrivate (String value) { + return value == null || isNoOrPrivate(value); } /** From 44a9cad8aff88d0f3f1916d775c757aed1aec590 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 28 Dec 2022 19:43:46 +0800 Subject: [PATCH 25/46] never start more than half the remaining capacity Allows config.maxWorkers() and MAX_WORKERS_PER_CATEGORY to be more freely set on staging. --- .../analysis/components/broker/Broker.java | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index ee1193b3d..99c305db1 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -1,6 +1,5 @@ package com.conveyal.analysis.components.broker; -import com.conveyal.analysis.AnalysisServerException; import com.conveyal.analysis.components.Component; import com.conveyal.analysis.components.WorkerLauncher; import com.conveyal.analysis.components.eventbus.ErrorEvent; @@ -249,31 +248,49 @@ public void createOnDemandWorkerInCategory(WorkerCategory category, WorkerTags w */ public void createWorkersInCategory (WorkerCategory category, WorkerTags workerTags, int nOnDemand, int nSpot) { + // Log error messages rather than throwing exceptions, as this code often runs in worker poll handlers. + // Throwing an exception there would not report any useful infomation to anyone. + if (config.offline()) { - LOG.info("Work offline enabled, not creating workers for {}", category); + LOG.info("Work offline enabled, not creating workers for {}.", category); return; } - if (nOnDemand < 0 || nSpot < 0){ - LOG.info("Negative number of workers requested, not starting any"); + if (nOnDemand < 0 || nSpot < 0) { + LOG.error("Negative number of workers requested, not starting any."); return; } - // Backoff: reduce the nSpot requested when the number of already running workers starts approaching the - // configured maximum - if (workerCatalog.totalWorkerCount() * 2 > config.maxWorkers()) { - nSpot = Math.min(nSpot, (config.maxWorkers() - workerCatalog.totalWorkerCount()) / 2); - LOG.info("Worker pool over half of maximum size. Number of new spot instances set to {}", nSpot); + final int nRequested = nOnDemand + nSpot; + if (nRequested <= 0) { + LOG.error("No workers requested, not starting any."); + return; + } + + // Zeno's worker pool management: never start more than half the remaining capacity. + final int remainingCapacity = config.maxWorkers() - workerCatalog.totalWorkerCount(); + final int maxToStart = remainingCapacity / 2; + if (maxToStart <= 0) { + LOG.error("Due to capacity limiting, not starting any workers."); + return; } - if (workerCatalog.totalWorkerCount() + nOnDemand + nSpot >= config.maxWorkers()) { - String message = String.format( - "Maximum of %d workers already started, not starting more;" + - "jobs will not complete on %s", - config.maxWorkers(), - category + if (nRequested > maxToStart) { + LOG.warn("Request for {} workers is more than half the remaining worker pool capacity.", nRequested); + nSpot = maxToStart; + nOnDemand = 0; + LOG.warn("Lowered to {} on-demand and {} spot workers.", nOnDemand, nSpot); + } + + // Just an assertion for consistent state - this should never happen. + // Re-sum nOnDemand + nSpot here instead of using nTotal, as they may have been revised. + if (workerCatalog.totalWorkerCount() + nOnDemand + nSpot > config.maxWorkers()) { + LOG.error( + "Starting workers would exceed the maximum capacity of {}. Jobs may stall on {}.", + config.maxWorkers(), + category ); - throw AnalysisServerException.forbidden(message); + return; } // If workers have already been started up, don't repeat the operation. From 6e7c673ea7ff5371a134f5d2d77f77942d9d20a9 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 28 Dec 2022 23:01:04 +0800 Subject: [PATCH 26/46] do not check for access=private --- .../com/conveyal/r5/streets/StreetLayer.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/r5/streets/StreetLayer.java b/src/main/java/com/conveyal/r5/streets/StreetLayer.java index 0d2dbbfe7..de7a7fd1f 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetLayer.java +++ b/src/main/java/com/conveyal/r5/streets/StreetLayer.java @@ -1079,10 +1079,17 @@ private static short speedToShort(Float speed) { * https://wiki.openstreetmap.org/wiki/Key:barrier#Values we see that most of the values represent things that are * easily passable, and even barrier=gate is implicitly openable unless tagged with access=no|private. * - * Even when access=no|private, there are frequently exceptions for single modes. We don't yet handle single-mode + * Even when access=no, there are frequently exceptions for single modes. We don't yet handle single-mode * barriers, so we want to default to the preexisting behavior of allowing passage (and relying only on edge * traversal permissions) whenever we see exception tags that aren't clearly blocking access. * See https://taginfo.openstreetmap.org/keys/foot#values. + * + * We don't actually check for access|foot|bicycle=private because a private origin or destination area (gated + * housing or secure work facility) is still accessible to a person who lives or works there. We realized this + * because https://www.vtvzuidbuurt.nl/ near Vlaardingen in the Netherlands saw a huge drop in accessibility when + * we checked for no|private. This is a large housing complex with only two gates, both marked private. + * + * Ideally such areas would be treated as no-through-traffic but that would involve more tricky heuristics. */ private static boolean isImpassable (Node node) { // This code is hit millions of times so we want to bypass it as much as possible. @@ -1093,22 +1100,22 @@ private static boolean isImpassable (Node node) { if (node.hasTag("entrance", "emergency")) { return true; } - if (isNoOrPrivate(node.getTag("access"))) { - // This node is explicitly decared inaccessible or private, but there might be exceptions. + if (isNo(node.getTag("access"))) { + // This node is explicitly decared inaccessible, but there might be exceptions. // Err on the side of using the existing code path by returning false. // Consider the node impassable only when all mode-specific exception tags are missing or clearly negative. - return isNullNoOrPrivate(node.getTag("foot")) && isNullNoOrPrivate(node.getTag("bicycle")); + return isNullOrNo(node.getTag("foot")) && isNullOrNo(node.getTag("bicycle")); } // As a default, err on the side of returning false, which will maintain the preexisting code path. return false; } - private static boolean isNoOrPrivate (String value) { - return "no".equals(value) || "private".equals(value); + private static boolean isNo(String value) { + return "no".equals(value); } - private static boolean isNullNoOrPrivate (String value) { - return value == null || isNoOrPrivate(value); + private static boolean isNullOrNo(String value) { + return value == null || isNo(value); } /** From e69e298982de9586c62570a050be9ba53a1c499a Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 28 Dec 2022 23:15:14 +0800 Subject: [PATCH 27/46] Fix typo in comment Co-authored-by: Anson Stewart --- .../java/com/conveyal/analysis/components/broker/Broker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index 99c305db1..d8895e6e3 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -249,7 +249,7 @@ public void createOnDemandWorkerInCategory(WorkerCategory category, WorkerTags w public void createWorkersInCategory (WorkerCategory category, WorkerTags workerTags, int nOnDemand, int nSpot) { // Log error messages rather than throwing exceptions, as this code often runs in worker poll handlers. - // Throwing an exception there would not report any useful infomation to anyone. + // Throwing an exception there would not report any useful information to anyone. if (config.offline()) { LOG.info("Work offline enabled, not creating workers for {}.", category); From 0087e0d899bc90257bcc429074768fa55787ac6c Mon Sep 17 00:00:00 2001 From: ansons Date: Thu, 29 Dec 2022 14:30:05 -0500 Subject: [PATCH 28/46] Increase autoscaling limits for freeform analyses --- .../java/com/conveyal/analysis/components/broker/Broker.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index d8895e6e3..1eeb93641 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -530,8 +530,9 @@ private void requestExtraWorkersIfAppropriate(Job job) { // Do not exceed the limit on workers per category TODO add similar limit per accessGroup or user targetWorkerTotal = Math.min(targetWorkerTotal, MAX_WORKERS_PER_CATEGORY); - // Guardrail until freeform pointsets are tested more thoroughly - if (job.templateTask.originPointSet != null) targetWorkerTotal = Math.min(targetWorkerTotal, 5); + // Guardrails until freeform pointsets are tested more thoroughly + if (job.templateTask.originPointSet != null) targetWorkerTotal = Math.min(targetWorkerTotal, 80); + if (job.templateTask.includePathResults) targetWorkerTotal = Math.min(targetWorkerTotal, 20); int nSpot = targetWorkerTotal - categoryWorkersAlreadyRunning; createWorkersInCategory(job.workerCategory, job.workerTags, 0, nSpot); } From c6bcd2810ae7f29bda0444ab637e64e453df4f47 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Mon, 23 Jan 2023 02:38:42 +0100 Subject: [PATCH 29/46] Allow UUIDs without hyphens (#854) We used UUIDs without hyphens for a period in the past. Modify the BrokerController to allow those. Migrating would be ideal. But migrating IDs is not simple. We have a bigger DB migration planned in the future and will migrate IDs to a single type then. --- .../controllers/BrokerController.java | 42 +++++++++++------ .../controllers/ArgumentValidationTest.java | 45 +++++++++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java diff --git a/src/main/java/com/conveyal/analysis/controllers/BrokerController.java b/src/main/java/com/conveyal/analysis/controllers/BrokerController.java index b59c71037..409180fef 100644 --- a/src/main/java/com/conveyal/analysis/controllers/BrokerController.java +++ b/src/main/java/com/conveyal/analysis/controllers/BrokerController.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.mongodb.QueryBuilder; +import org.apache.commons.math3.analysis.function.Exp; import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; @@ -134,10 +135,10 @@ private Object singlePoint(Request request, Response response) { AnalysisRequest analysisRequest = objectFromRequestBody(request, AnalysisRequest.class); // Some parameters like regionId weren't sent by older frontends. Fail fast on missing parameters. - checkUuidParameter(analysisRequest.regionId, "region ID"); - checkUuidParameter(analysisRequest.projectId, "project ID"); - checkUuidParameter(analysisRequest.bundleId, "bundle ID"); - checkUuidParameters(analysisRequest.modificationIds, "modification IDs"); + checkIdParameter(analysisRequest.regionId, "region ID"); + checkIdParameter(analysisRequest.projectId, "project ID"); + checkIdParameter(analysisRequest.bundleId, "bundle ID"); + checkIdParameters(analysisRequest.modificationIds, "modification IDs"); checkNotNull(analysisRequest.workerVersion, "Worker version must be provided in request."); // Transform the analysis UI/backend task format into a slightly different type for R5 workers. @@ -388,30 +389,45 @@ private static void enforceAdmin (Request request) { } } + /** Factor out exception creation for readability/repetition. */ + private static IllegalArgumentException uuidException (String fieldName) { + return new IllegalArgumentException(String.format("The %s does not appear to be an ObjectId or UUID.", fieldName)); + } + /** * Validate a request parameter that is expected to be a non-null String containing a Mongo ObjectId or a - * UUID converted to a string in the standard way. + * UUID converted to a string, or a UUID converted to a string with the hyphens removed. * @param name a human-readable name for the parameter being validated, for substitution into error messages. * @throws IllegalArgumentException if the parameter fails any of these conditions. */ - public static void checkUuidParameter (String parameter, String name) { + public static void checkIdParameter(String parameter, String name) { if (parameter == null) { throw new IllegalArgumentException(String.format("The %s is not set in the request.", name)); } - // Should be either 24 hex digits (Mongo ID) or 32 hex digits with dashes (UUID) + // Should be either 24 hex digits (Mongo ID), 32 hex digits with dashes (UUID), or 32 hex digits without dashes if (ObjectId.isValid(parameter)) { return; } - try { - UUID.fromString(parameter); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException(String.format("The %s does not appear to be an ObjectId or UUID.", name)); + if (parameter.length() == 36) { + try { + UUID.fromString(parameter); + } catch (IllegalArgumentException e) { + throw uuidException(name); + } + } else if (parameter.length() == 32) { + for (char c : parameter.toCharArray()) { + if (Character.digit(c, 16) == -1) { + throw uuidException(name); + } + } + } else { + throw uuidException(name); } } - public static void checkUuidParameters (Iterable parameters, String name) { + public static void checkIdParameters(Iterable parameters, String name) { for (String parameter: parameters) { - checkUuidParameter(parameter, name); + checkIdParameter(parameter, name); } } diff --git a/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java b/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java new file mode 100644 index 000000000..a56769fba --- /dev/null +++ b/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java @@ -0,0 +1,45 @@ +package com.conveyal.analysis.controllers; + +import com.conveyal.analysis.components.broker.Broker; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.UUID; + +/** + * Tests that check how parameters received over the HTTP API are validated. + * These validators should be fairly strict about what they accept, and should not tolerate the presence of things + * like semicolons or double dashes that indicate attempts to corrupt or gain access to database contents. + * + * Arguably we should have another layer of input sanitization that not only refuses but logs anything that contains + * characters or substrings that could be associated with an attempted attack, and that same validator should be + * applied to every input (perhaps called from every other input validator). + */ +public class ArgumentValidationTest { + + @Test + void testIdValidation () { + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("hello", "param"); + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("Robert'); DROP TABLE Students;--", "param"); + // https://xkcd.com/327/ + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("0123456789012345", "param"); + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("0123456789ABCDEF67890ZZZZ5678901", "param"); + }); + Assertions.assertDoesNotThrow(() -> { + BrokerController.checkIdParameter("0123456789abcDEF6789012345678901", "param"); + }); + Assertions.assertDoesNotThrow(() -> { + String validUuid = UUID.randomUUID().toString(); + BrokerController.checkIdParameter(validUuid, "param"); + }); + } + + +} From 38290e93571b5a699d4ccad4a926beff361da43a Mon Sep 17 00:00:00 2001 From: ansons Date: Wed, 25 Jan 2023 00:59:35 -0500 Subject: [PATCH 30/46] fix(paths): return correct route info Fixes #855 --- .../r5/analyst/cluster/PathResultSummary.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java b/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java index 812592eb2..ef9df4ae2 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java @@ -2,6 +2,7 @@ import com.conveyal.r5.analyst.StreetTimesAndModes; import com.conveyal.r5.transit.TransitLayer; +import com.conveyal.r5.transit.path.RouteSequence; import com.conveyal.r5.transit.path.StopSequence; import gnu.trove.list.TIntList; import gnu.trove.list.array.TIntArrayList; @@ -49,8 +50,8 @@ public PathResultSummary( this.iterations.add(iterationDetails); } List transitLegs = new ArrayList<>(); - for (int routeIndex = 0; routeIndex < pathTemplate.routes.size(); routeIndex++) { - transitLegs.add(new TransitLeg(transitLayer, pathTemplate.stopSequence, routeIndex)); + for (int legIndex = 0; legIndex < pathTemplate.routes.size(); legIndex++) { + transitLegs.add(new TransitLeg(transitLayer, pathTemplate, legIndex)); } itineraries.add(new Itinerary( pathTemplate.stopSequence.access, @@ -115,20 +116,21 @@ static class TransitLeg { public TransitLeg( TransitLayer transitLayer, - StopSequence stopSequence, - int routeIndex + RouteSequence routeSequence, + int legIndex ) { - var routeInfo = transitLayer.routes.get(routeIndex); + var routeInfo = transitLayer.routes.get(routeSequence.routes.get(legIndex)); routeId = routeInfo.route_id; routeName = routeInfo.getName(); + StopSequence stopSequence = routeSequence.stopSequence; - rideTimeSeconds = stopSequence.rideTimesSeconds.get(routeIndex); + rideTimeSeconds = stopSequence.rideTimesSeconds.get(legIndex); - int boardStopIndex = stopSequence.boardStops.get(routeIndex); + int boardStopIndex = stopSequence.boardStops.get(legIndex); boardStopId = getStopId(transitLayer, boardStopIndex); boardStopName = transitLayer.stopNames.get(boardStopIndex); - int alightStopIndex = stopSequence.alightStops.get(routeIndex); + int alightStopIndex = stopSequence.alightStops.get(legIndex); alightStopId = getStopId(transitLayer, alightStopIndex); alightStopName = transitLayer.stopNames.get(alightStopIndex); } From c483a7703918b2b324bd57efd2a11b9e8c89c7a5 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 1 Feb 2023 23:27:29 +0800 Subject: [PATCH 31/46] bail out of ways that reference undefined nodes fixes #851 --- .../com/conveyal/r5/streets/StreetLayer.java | 4 +++ .../conveyal/r5/streets/StreetLayerTest.java | 20 ++++++++++++++ .../com/conveyal/r5/streets/missing-nodes.pbf | Bin 0 -> 592 bytes .../com/conveyal/r5/streets/missing-nodes.xml | 25 ++++++++++++++++++ 4 files changed, 49 insertions(+) create mode 100644 src/test/resources/com/conveyal/r5/streets/missing-nodes.pbf create mode 100644 src/test/resources/com/conveyal/r5/streets/missing-nodes.xml diff --git a/src/main/java/com/conveyal/r5/streets/StreetLayer.java b/src/main/java/com/conveyal/r5/streets/StreetLayer.java index de7a7fd1f..3d781b41b 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetLayer.java +++ b/src/main/java/com/conveyal/r5/streets/StreetLayer.java @@ -310,6 +310,10 @@ void loadFromOsm (OSM osm, boolean removeIslands, boolean saveVertexIndex) { for (int n = 1; n < way.nodes.length; n++) { long nodeId = way.nodes[n]; Node node = osm.nodes.get(nodeId); + if (node == null) { + LOG.warn("Bailing out of OSM way {} that references an undefined node.", entry.getKey()); + break; + } final boolean intersection = osm.intersectionNodes.contains(way.nodes[n]); final boolean lastNode = (n == (way.nodes.length - 1)); if (intersection || lastNode || isImpassable(node)) { diff --git a/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java b/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java index a062bec61..37e7f0c24 100644 --- a/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java +++ b/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java @@ -14,6 +14,7 @@ import java.util.EnumSet; import java.util.List; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -400,4 +401,23 @@ private int connectedVertices(StreetLayer sl, int vertexId) { return r.getReachedVertices().size(); } + /** + * We have decided to tolerate OSM data containing ways that reference missing nodes, because geographic extract + * processes often produce data like this. Load a file containing a way that ends with some missing nodes + * and make sure no exception occurs. The input must contain ways creating intersections such that at least one + * edge is produced, as later steps expect the edge store to be non-empty. The PBF fixture for this test is derived + * from the hand-tweaked XML file of the same name using osmconvert. + */ + @Test + public void testMissingNodes () { + OSM osm = new OSM(null); + osm.intersectionDetection = true; + osm.readFromUrl(StreetLayerTest.class.getResource("missing-nodes.pbf").toString()); + assertDoesNotThrow(() -> { + StreetLayer sl = new StreetLayer(); + sl.loadFromOsm(osm, true, true); + sl.buildEdgeLists(); + }); + } + } diff --git a/src/test/resources/com/conveyal/r5/streets/missing-nodes.pbf b/src/test/resources/com/conveyal/r5/streets/missing-nodes.pbf new file mode 100644 index 0000000000000000000000000000000000000000..6e927f26cd3189882934b0e58af59ee2889347c2 GIT binary patch literal 592 zcmV-W0m~dw0!f1hpjt@(sB_4b@ z(Kwk|;X%WK|C5-N`234=gOf8-a}#yL4D`&DxLi{6ic|gaQ&Nky1cUR7O7uc13sU1t zGE(#6Jzbg@1@nt@lk@Y+Qj1Cy4D>AY3=O&%RWeFS3as??%gf94@(Y0aONvrcOL7wn z^zw_+^%Dy+^?@b>0HDnA5^xf33J=_-&G`ZT%eNy2nzjA43o11V1OWj7 z0TK?DtmAD;0s;U6LJNw` z?1KcQN7V$G3`z}%!{T&^`~dO$p`>XdpI~x7xZiZ literal 0 HcmV?d00001 diff --git a/src/test/resources/com/conveyal/r5/streets/missing-nodes.xml b/src/test/resources/com/conveyal/r5/streets/missing-nodes.xml new file mode 100644 index 000000000..0a9cf4616 --- /dev/null +++ b/src/test/resources/com/conveyal/r5/streets/missing-nodes.xml @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file From f36dec3a5005934d62015d37f5c2e54f4fa4aa0f Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Thu, 2 Feb 2023 08:05:01 +0100 Subject: [PATCH 32/46] Make `OsmCache.getKey` a static method (#857) BundleController now no longer depends on `OSMCache` as a component. --- .../com/conveyal/analysis/controllers/BundleController.java | 5 +---- src/main/java/com/conveyal/r5/streets/OSMCache.java | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/controllers/BundleController.java b/src/main/java/com/conveyal/analysis/controllers/BundleController.java index 269189407..b7fc71cc5 100644 --- a/src/main/java/com/conveyal/analysis/controllers/BundleController.java +++ b/src/main/java/com/conveyal/analysis/controllers/BundleController.java @@ -67,14 +67,11 @@ public class BundleController implements HttpController { private final FileStorage fileStorage; private final GTFSCache gtfsCache; - // FIXME The backend appears to use an osmcache purely to get a file key at which to store incoming OSM. Refactor. - private final OSMCache osmCache; private final TaskScheduler taskScheduler; public BundleController (BackendComponents components) { this.fileStorage = components.fileStorage; this.gtfsCache = components.gtfsCache; - this.osmCache = components.osmCache; this.taskScheduler = components.taskScheduler; } @@ -176,7 +173,7 @@ private Bundle create (Request req, Response res) { osm.close(); checkWgsEnvelopeSize(osmBounds, "OSM data"); // Store the source OSM file. Note that we're not storing the derived MapDB file here. - fileStorage.moveIntoStorage(osmCache.getKey(bundle.osmId), fi.getStoreLocation()); + fileStorage.moveIntoStorage(OSMCache.getKey(bundle.osmId), fi.getStoreLocation()); } if (bundle.feedGroupId == null) { diff --git a/src/main/java/com/conveyal/r5/streets/OSMCache.java b/src/main/java/com/conveyal/r5/streets/OSMCache.java index 77df063ae..e51a6f07c 100644 --- a/src/main/java/com/conveyal/r5/streets/OSMCache.java +++ b/src/main/java/com/conveyal/r5/streets/OSMCache.java @@ -33,11 +33,11 @@ public OSMCache (FileStorage fileStorage) { .maximumSize(10) .build(); - public String cleanId(String id) { + public static String cleanId(String id) { return id.replaceAll("[^A-Za-z0-9]", "-"); } - public FileStorageKey getKey (String id) { + public static FileStorageKey getKey (String id) { // FIXME Transforming IDs each time they're used seems problematic. They should probably only be validated here. String cleanId = cleanId(id); return new FileStorageKey(FileCategory.BUNDLES, cleanId + ".pbf"); From 1f5433b880c505f5d35e7c8065dec198b5645cc0 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 3 Feb 2023 16:17:58 +0800 Subject: [PATCH 33/46] additional street modes in TransportNetworkConfig added comments explaining how only walk stop-to-vertex tables are kept --- .../cluster/TransportNetworkConfig.java | 15 +++++ .../conveyal/r5/streets/EgressCostTable.java | 20 +++--- .../com/conveyal/r5/transit/TransitLayer.java | 4 +- .../conveyal/r5/transit/TransportNetwork.java | 19 +++--- .../r5/transit/TransportNetworkCache.java | 61 ++++++++++++------- 5 files changed, 78 insertions(+), 41 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java b/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java index 54f11ee8c..924939bb0 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java @@ -4,11 +4,13 @@ import com.conveyal.r5.analyst.scenario.Modification; import com.conveyal.r5.analyst.scenario.RasterCost; import com.conveyal.r5.analyst.scenario.ShapefileLts; +import com.conveyal.r5.profile.StreetMode; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.databind.annotation.JsonSerialize; import java.util.List; +import java.util.Set; /** * All inputs and options that describe how to build a particular transport network (except the serialization version). @@ -39,4 +41,17 @@ public class TransportNetworkConfig { /** A list of _R5_ modifications to apply during network build. May be null. */ public List modifications; + /** + * Additional modes other than walk for which to pre-build a full regional grid and distance tables. + * When building a network, by default we build distance tables from transit stops to street vertices, to which we + * connect a grid covering the entire street network at the default zoom level. By default we do this only for the + * walk mode. Pre-building and serializing equivalent data structures for other modes allows workers to start up + * much faster in regional analyses. The work need only be done once when the first single-point worker to builds + * the network. Otherwise, hundreds of workers will each have to build these tables every time they start up. + * Some scenarios, such as those that affect the street layer, may still be slower to apply for modes listed here + * because some intermediate data (stop-to-vertex tables) are only retained for the walk mode. If this proves to be + * a problem it is a candidate for future optimization. + */ + public Set buildGridsForModes; + } diff --git a/src/main/java/com/conveyal/r5/streets/EgressCostTable.java b/src/main/java/com/conveyal/r5/streets/EgressCostTable.java index cb72a65e1..0d4e750b0 100644 --- a/src/main/java/com/conveyal/r5/streets/EgressCostTable.java +++ b/src/main/java/com/conveyal/r5/streets/EgressCostTable.java @@ -262,16 +262,16 @@ public EgressCostTable (LinkedPointSet linkedPointSet, ProgressListener progress GeometryUtils.expandEnvelopeFixed(envelopeAroundStop, linkingDistanceLimitMeters); if (streetMode == StreetMode.WALK) { - // Walking distances from stops to street vertices are saved in the TransitLayer. - // Get the pre-computed walking distance table from the stop to the street vertices, - // then extend that table out from the street vertices to the points in this PointSet. - // TODO reuse the code that computes the walk tables at TransitLayer.buildOneDistanceTable() rather than - // duplicating it below for other modes. + // Distances from stops to street vertices are saved in the TransitLayer, but only for the walk mode. + // Get the pre-computed walking distance table from the stop to the street vertices, then extend that + // table out from the street vertices to the points in this PointSet. It may be possible to reuse the + // code that pre-computes walk tables at TransitLayer.buildOneDistanceTable() rather than duplicating + // it below for other (non-walk) modes. TIntIntMap distanceTableToVertices = transitLayer.stopToVertexDistanceTables.get(stopIndex); return distanceTableToVertices == null ? null : linkedPointSet.extendDistanceTableToPoints(distanceTableToVertices, envelopeAroundStop); } else { - + // For non-walk modes perform a search from each stop, as stop-to-vertex tables are not precomputed. Geometry egressArea = null; // If a pickup delay modification is present for this street mode, egressStopDelaysSeconds is @@ -301,14 +301,14 @@ public EgressCostTable (LinkedPointSet linkedPointSet, ProgressListener progress LOG.warn("Stop unlinked, cannot build distance table: {}", stopIndex); return null; } - // TODO setting the origin point of the router to the stop vertex does not work. - // This is probably because link edges do not allow car traversal. We could traverse them. - // As a stopgap we perform car linking at the geographic coordinate of the stop. + // Setting the origin point of the router to the stop vertex (as follows) does not work. // sr.setOrigin(vertexId); + // This is probably because link edges do not allow car traversal. We could traverse them. + // As a workaround we perform car linking at the geographic coordinate of the stop. VertexStore.Vertex vertex = linkedPointSet.streetLayer.vertexStore.getCursor(vertexId); sr.setOrigin(vertex.getLat(), vertex.getLon()); - // WALK is handled above, this block is exhaustively handling all other modes. + // WALK is handled in the if clause above, this else block is exhaustively handling all other modes. if (streetMode == StreetMode.BICYCLE) { sr.distanceLimitMeters = linkingDistanceLimitMeters; } else if (streetMode == StreetMode.CAR) { diff --git a/src/main/java/com/conveyal/r5/transit/TransitLayer.java b/src/main/java/com/conveyal/r5/transit/TransitLayer.java index e140b0d89..b7c5247ca 100644 --- a/src/main/java/com/conveyal/r5/transit/TransitLayer.java +++ b/src/main/java/com/conveyal/r5/transit/TransitLayer.java @@ -535,11 +535,13 @@ public void rebuildTransientIndexes () { } /** - * Run a distance-constrained street search from every transit stop in the graph. + * Run a distance-constrained street search from every transit stop in the graph using the walk mode. * Store the distance to every reachable street vertex for each of these origin stops. * If a scenario has been applied, we need to build tables for any newly created stops and any stops within * transfer distance or access/egress distance of those new stops. In that case a rebuildZone geometry should be * supplied. If rebuildZone is null, a complete rebuild of all tables will occur for all stops. + * Note, this rebuilds for the WALK MODE ONLY. The network only has a field for retaining walk distance tables. + * This is a candidate for optimization if car or bicycle scenarios are slow to apply. * @param rebuildZone the zone within which to rebuild tables in FIXED-POINT DEGREES, or null to build all tables. */ public void buildDistanceTables(Geometry rebuildZone) { diff --git a/src/main/java/com/conveyal/r5/transit/TransportNetwork.java b/src/main/java/com/conveyal/r5/transit/TransportNetwork.java index c68a8f166..3e3cb3720 100644 --- a/src/main/java/com/conveyal/r5/transit/TransportNetwork.java +++ b/src/main/java/com/conveyal/r5/transit/TransportNetwork.java @@ -25,6 +25,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Stream; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; @@ -263,14 +264,14 @@ public static InputFileType forFile(File file) { } /** - * For Analysis purposes, build an efficient implicit grid PointSet for this TransportNetwork. Then, for any modes - * supplied, we also build a linkage that is held permanently in the GridPointSet. This method is called when a - * network is first built. - * The resulting grid PointSet will cover the entire street network layer of this TransportNetwork, which should - * include every point we can route from or to. Any other destination grid (for the same mode, walking) can be made - * as a subset of this one since it includes every potentially accessible point. + * Build a grid PointSet covering the entire street network layer of this TransportNetwork, which should include + * every point we can route from or to. Then for all requested modes build a linkage that is held in the + * GridPointSet. This method is called when a network is first built so these linkages are serialized with it. + * Any other destination grid (at least for the same modes) can be made as a subset of this one since it includes + * every potentially accessible point. Destination grids for other modes will be made on demand, which is a slow + * operation that can occupy hundreds of workers for long periods of time when a regional analysis starts up. */ - public void rebuildLinkedGridPointSet(StreetMode... modes) { + public void rebuildLinkedGridPointSet(Iterable modes) { if (fullExtentGridPointSet != null) { throw new RuntimeException("Linked grid pointset was built more than once."); } @@ -280,6 +281,10 @@ public void rebuildLinkedGridPointSet(StreetMode... modes) { } } + public void rebuildLinkedGridPointSet(StreetMode... modes) { + rebuildLinkedGridPointSet(Set.of(modes)); + } + //TODO: add transit stops to envelope public Envelope getEnvelope() { return streetLayer.getEnvelope(); diff --git a/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java b/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java index d93891ba1..b7f1f1d8f 100644 --- a/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java +++ b/src/main/java/com/conveyal/r5/transit/TransportNetworkCache.java @@ -19,6 +19,7 @@ import com.conveyal.r5.streets.StreetLayer; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; +import com.google.common.collect.Sets; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -158,28 +159,52 @@ private static FileStorageKey getR5NetworkFileStorageKey (String networkId) { return new FileStorageKey(BUNDLES, getR5NetworkFilename(networkId)); } + /** @return the network configuration (AKA manifest) for the given network ID, or null if no config file exists. */ + private TransportNetworkConfig loadNetworkConfig (String networkId) { + FileStorageKey configFileKey = new FileStorageKey(BUNDLES, getNetworkConfigFilename(networkId)); + if (!fileStorage.exists(configFileKey)) { + return null; + } + File configFile = fileStorage.getFile(configFileKey); + try { + // Use lenient mapper to mimic behavior in objectFromRequestBody. + return JsonUtilities.lenientObjectMapper.readValue(configFile, TransportNetworkConfig.class); + } catch (IOException e) { + throw new RuntimeException("Error reading TransportNetworkConfig. Does it contain new unrecognized fields?", e); + } + } + /** * If we did not find a cached network, build one from the input files. Should throw an exception rather than * returning null if for any reason it can't finish building one. */ private @Nonnull TransportNetwork buildNetwork (String networkId) { TransportNetwork network; - FileStorageKey networkConfigKey = new FileStorageKey(BUNDLES, GTFSCache.cleanId(networkId) + ".json"); - if (fileStorage.exists(networkConfigKey)) { - network = buildNetworkFromConfig(networkId); - } else { - LOG.warn("Detected old-format bundle stored as single ZIP file"); + TransportNetworkConfig networkConfig = loadNetworkConfig(networkId); + if (networkConfig == null) { + // The switch to use JSON manifests instead of zips occurred in 32a1aebe in July 2016. + // Over six years have passed, buildNetworkFromBundleZip is deprecated and could probably be removed. + LOG.warn("No network config (aka manifest) found. Assuming old-format network inputs bundle stored as a single ZIP file."); network = buildNetworkFromBundleZip(networkId); + } else { + network = buildNetworkFromConfig(networkConfig); } network.scenarioId = networkId; - // Networks created in TransportNetworkCache are going to be used for analysis work. Pre-compute distance tables - // from stops to street vertices, then pre-build a linked grid pointset for the whole region. These linkages - // should be serialized along with the network, which avoids building them when an analysis worker starts. - // The linkage we create here will never be used directly, but serves as a basis for scenario linkages, making - // analysis much faster to start up. + // Pre-compute distance tables from stops out to street vertices, then pre-build a linked grid pointset for the + // whole region covered by the street network. These tables and linkages will be serialized along with the + // network, which avoids building them when every analysis worker starts. The linkage we create here will never + // be used directly, but serves as a basis for scenario linkages, making analyses much faster to start up. + // Note, this retains stop-to-vertex distances for the WALK MODE ONLY, even when they are produced as + // intermediate results while building linkages for other modes. + // This is a candidate for optimization if car or bicycle scenarios are slow to apply. network.transitLayer.buildDistanceTables(null); - network.rebuildLinkedGridPointSet(StreetMode.WALK); + + Set buildGridsForModes = Sets.newHashSet(StreetMode.WALK); + if (networkConfig != null && networkConfig.buildGridsForModes != null) { + buildGridsForModes.addAll(networkConfig.buildGridsForModes); + } + network.rebuildLinkedGridPointSet(buildGridsForModes); // Cache the serialized network on the local filesystem and mirror it to any remote storage. try { @@ -247,17 +272,7 @@ private TransportNetwork buildNetworkFromBundleZip (String networkId) { * This describes the locations of files used to create a bundle, as well as options applied at network build time. * It contains the unique IDs of the GTFS feeds and OSM extract. */ - private TransportNetwork buildNetworkFromConfig (String networkId) { - FileStorageKey configFileKey = new FileStorageKey(BUNDLES, getNetworkConfigFilename(networkId)); - File configFile = fileStorage.getFile(configFileKey); - TransportNetworkConfig config; - - try { - // Use lenient mapper to mimic behavior in objectFromRequestBody. - config = JsonUtilities.lenientObjectMapper.readValue(configFile, TransportNetworkConfig.class); - } catch (IOException e) { - throw new RuntimeException("Error reading TransportNetworkConfig. Does it contain new unrecognized fields?", e); - } + private TransportNetwork buildNetworkFromConfig (TransportNetworkConfig config) { // FIXME duplicate code. All internal building logic should be encapsulated in a method like // TransportNetwork.build(osm, gtfs1, gtfs2...) // We currently have multiple copies of it, in buildNetworkFromConfig and buildNetworkFromBundleZip so you've @@ -265,7 +280,7 @@ private TransportNetwork buildNetworkFromConfig (String networkId) { // Maybe we should just completely deprecate bundle ZIPs and remove those code paths. TransportNetwork network = new TransportNetwork(); - network.scenarioId = networkId; + network.streetLayer = new StreetLayer(); network.streetLayer.loadFromOsm(osmCache.get(config.osmId)); From edacc133a85ef75588316c7400a096609c42cbe9 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 3 Feb 2023 20:34:53 +0800 Subject: [PATCH 34/46] include street mode in linkage/egress log messages --- src/main/java/com/conveyal/r5/streets/EgressCostTable.java | 6 +++--- src/main/java/com/conveyal/r5/streets/LinkedPointSet.java | 4 ++-- src/main/java/com/conveyal/r5/transit/TransitLayer.java | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/conveyal/r5/streets/EgressCostTable.java b/src/main/java/com/conveyal/r5/streets/EgressCostTable.java index 0d4e750b0..60bf5e336 100644 --- a/src/main/java/com/conveyal/r5/streets/EgressCostTable.java +++ b/src/main/java/com/conveyal/r5/streets/EgressCostTable.java @@ -209,7 +209,7 @@ public EgressCostTable (LinkedPointSet linkedPointSet, ProgressListener progress rebuildZone = linkedPointSet.streetLayer.scenarioEdgesBoundingGeometry(linkingDistanceLimitMeters); } - LOG.info("Creating EgressCostTables from each transit stop to PointSet points."); + LOG.info("Creating EgressCostTables from each transit stop to PointSet points for mode {}.", streetMode); if (rebuildZone != null) { LOG.info("Selectively computing tables for only those stops that might be affected by the scenario."); } @@ -232,9 +232,9 @@ public EgressCostTable (LinkedPointSet linkedPointSet, ProgressListener progress progressListener.beginTask(taskDescription, nStops); final LambdaCounter computeCounter = new LambdaCounter(LOG, nStops, computeLogFrequency, - "Computed new stop -> point tables for {} of {} transit stops."); + String.format("Computed new stop-to-point tables from {} of {} transit stops for mode %s.", streetMode)); final LambdaCounter copyCounter = new LambdaCounter(LOG, nStops, copyLogFrequency, - "Copied unchanged stop -> point tables for {} of {} transit stops."); + String.format("Copied unchanged stop-to-point tables from {} of {} transit stops for mode %s.", streetMode)); // Create a distance table from each transit stop to the points in this PointSet in parallel. // Each table is a flattened 2D array. Two values for each point reachable from this stop: (pointIndex, cost) // When applying a scenario, keep the existing distance table for those stops that could not be affected. diff --git a/src/main/java/com/conveyal/r5/streets/LinkedPointSet.java b/src/main/java/com/conveyal/r5/streets/LinkedPointSet.java index c529ced10..01a51a43d 100644 --- a/src/main/java/com/conveyal/r5/streets/LinkedPointSet.java +++ b/src/main/java/com/conveyal/r5/streets/LinkedPointSet.java @@ -135,7 +135,7 @@ public class LinkedPointSet implements Serializable { * the same pointSet and streetMode as the preceding arguments. */ public LinkedPointSet (PointSet pointSet, StreetLayer streetLayer, StreetMode streetMode, LinkedPointSet baseLinkage) { - LOG.info("Linking pointset to street network..."); + LOG.info("Linking pointset to street network for mode {}...", streetMode); this.pointSet = pointSet; this.streetLayer = streetLayer; this.streetMode = streetMode; @@ -301,7 +301,7 @@ public synchronized EgressCostTable getEgressCostTable () { */ private void linkPointsToStreets (boolean all) { LambdaCounter linkCounter = new LambdaCounter(LOG, pointSet.featureCount(), 10000, - "Linked {} of {} PointSet points to streets."); + String.format("Linked {} of {} PointSet points to streets for mode %s.", streetMode)); // Construct a geometry around any edges added by the scenario, or null if there are no added edges. // As it is derived from edge geometries this is a fixed-point geometry and must be intersected with the same. diff --git a/src/main/java/com/conveyal/r5/transit/TransitLayer.java b/src/main/java/com/conveyal/r5/transit/TransitLayer.java index b7c5247ca..871491ff2 100644 --- a/src/main/java/com/conveyal/r5/transit/TransitLayer.java +++ b/src/main/java/com/conveyal/r5/transit/TransitLayer.java @@ -546,7 +546,7 @@ public void rebuildTransientIndexes () { */ public void buildDistanceTables(Geometry rebuildZone) { - LOG.info("Finding distances from transit stops to street vertices."); + LOG.info("Pre-computing distances from transit stops to street vertices (WALK mode only)."); if (rebuildZone != null) { LOG.info("Selectively finding distances for only those stops potentially affected by scenario application."); } From f2d7c32288390f37a6144f4a1b91c0e57a6d6c21 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Tue, 7 Feb 2023 10:46:57 +0800 Subject: [PATCH 35/46] clarify comment on new config option --- .../com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java b/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java index 924939bb0..012e3204a 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/TransportNetworkConfig.java @@ -42,7 +42,7 @@ public class TransportNetworkConfig { public List modifications; /** - * Additional modes other than walk for which to pre-build a full regional grid and distance tables. + * Additional modes other than walk for which to pre-build large data structures (grid linkage and cost tables). * When building a network, by default we build distance tables from transit stops to street vertices, to which we * connect a grid covering the entire street network at the default zoom level. By default we do this only for the * walk mode. Pre-building and serializing equivalent data structures for other modes allows workers to start up From 40c4d758f6eda891526c751c973eab063abb2bdb Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 15 Mar 2023 18:44:49 +0800 Subject: [PATCH 36/46] update build.gradle for gradle 8.x also fixed gradle 9.x deprecation warnings --- build.gradle | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/build.gradle b/build.gradle index fee0a0898..3ab0dbb0c 100644 --- a/build.gradle +++ b/build.gradle @@ -1,8 +1,8 @@ plugins { id 'java' - id 'com.github.johnrengelman.shadow' version '6.0.0' + id 'com.github.johnrengelman.shadow' version '8.1.0' id 'maven-publish' - id 'com.palantir.git-version' version '0.12.3' + id 'com.palantir.git-version' version '2.0.0' } group = 'com.conveyal' @@ -72,9 +72,11 @@ tasks.withType(JavaCompile) { options.encoding = 'UTF-8' } -// A task to copy all dependencies of the project into a single directory +// A task to copy all dependency JARs needed at runtime into a single directory task copyDependencies(type: Copy) { - from configurations.default + from(sourceSets.main.runtimeClasspath) { + include '*.jar' + } into 'dependencies' } @@ -82,7 +84,7 @@ task copyDependencies(type: Copy) { task runBackend (type: JavaExec) { dependsOn(build) classpath(sourceSets.main.runtimeClasspath) - main("com.conveyal.analysis.BackendMain") + mainClass = 'com.conveyal.analysis.BackendMain' } // Start up an analysis local backend from a shaded JAR and ask it to shut down immediately. @@ -91,7 +93,7 @@ task runBackend (type: JavaExec) { task testShadowJarRunnable(type: JavaExec) { dependsOn(shadowJar) classpath(shadowJar.archiveFile.get()) - main("com.conveyal.analysis.BackendMain") + mainClass = 'com.conveyal.analysis.BackendMain' jvmArgs("-Dconveyal.immediate.shutdown=true") } From 074c511fde485c750c4ef145f6a6a63cec0892e2 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 15 Mar 2023 19:02:04 +0800 Subject: [PATCH 37/46] use built-in gradle caching in setup-java action --- .github/workflows/gradle.yml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/.github/workflows/gradle.yml b/.github/workflows/gradle.yml index 22691e6d1..24f08b055 100644 --- a/.github/workflows/gradle.yml +++ b/.github/workflows/gradle.yml @@ -33,14 +33,11 @@ jobs: fetch-depth: 0 # Java setup step completes very fast, no need to run in a preconfigured docker container. - name: Set up JDK 11 - uses: actions/setup-java@v1 + uses: actions/setup-java@v3 with: java-version: 11 - - uses: actions/cache@v1 - id: cache - with: - path: ~/.gradle/caches - key: gradle-caches + distribution: temurin + cache: 'gradle' - name: Show version string run: gradle -q printVersion | head -n1 - name: Build and Test From e9d44e1997590d1387623102083e629973a2c795 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 16 Mar 2023 11:46:25 +0800 Subject: [PATCH 38/46] grow envelope from projected coords, fixes #833 --- .../datasource/ShapefileDataSourceIngester.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java b/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java index 03e61c6b7..21b1c06d4 100644 --- a/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java +++ b/src/main/java/com/conveyal/analysis/datasource/ShapefileDataSourceIngester.java @@ -44,24 +44,25 @@ public void ingest (File file, ProgressListener progressListener) { try { ShapefileReader reader = new ShapefileReader(file); // Iterate over all features to ensure file is readable, geometries are valid, and can be reprojected. - Envelope envelope = reader.wgs84Bounds(); - checkWgsEnvelopeSize(envelope, "Shapefile"); + // Note that the method reader.wgs84Bounds() transforms the envelope in projected coordinates to WGS84, + // which does not necessarily include all the points transformed individually. + Envelope envelope = new Envelope(); reader.wgs84Stream().forEach(f -> { - checkState(envelope.contains(((Geometry)f.getDefaultGeometry()).getEnvelopeInternal())); + envelope.expandToInclude(((Geometry)f.getDefaultGeometry()).getEnvelopeInternal()); }); + checkWgsEnvelopeSize(envelope, "Shapefile"); reader.close(); progressListener.increment(); dataSource.wgsBounds = Bounds.fromWgsEnvelope(envelope); dataSource.attributes = reader.attributes(); dataSource.geometryType = reader.geometryType(); dataSource.featureCount = reader.featureCount(); - dataSource.coordinateSystem = - reader.crs.getName().getCode(); - + dataSource.coordinateSystem = reader.crs.getName().getCode(); progressListener.increment(); } catch (FactoryException | TransformException e) { - throw new DataSourceException("Shapefile transform error. " + - "Try uploading an unprojected WGS84 (EPSG:4326) file.", e); + throw new DataSourceException( + "Shapefile transform error. Try uploading an unprojected WGS84 (EPSG:4326) file.", e + ); } catch (IOException e) { // ShapefileReader throws a checked IOException. throw new DataSourceException("Error parsing shapefile. Ensure the files you uploaded are valid.", e); From eb490b0a5f5c2ee88112aa26774a8067564c40c6 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 16 Mar 2023 16:12:55 +0800 Subject: [PATCH 39/46] check duplicate and missing GTFS entity keys the same method is applied for all entity types with simple string IDs addresses #792 --- .../gtfs/error/DuplicateKeyError.java | 4 +-- .../conveyal/gtfs/error/MissingKeyError.java | 22 ++++++++++++++++ .../java/com/conveyal/gtfs/model/Agency.java | 15 +++++++---- .../com/conveyal/gtfs/model/Calendar.java | 2 +- .../com/conveyal/gtfs/model/CalendarDate.java | 3 ++- .../java/com/conveyal/gtfs/model/Entity.java | 26 +++++++++++++++++-- .../conveyal/gtfs/model/FareAttribute.java | 2 +- .../java/com/conveyal/gtfs/model/Route.java | 7 ++++- .../java/com/conveyal/gtfs/model/Stop.java | 7 +++-- .../com/conveyal/gtfs/model/StopTime.java | 2 +- .../java/com/conveyal/gtfs/model/Trip.java | 7 ++++- 11 files changed, 80 insertions(+), 17 deletions(-) create mode 100644 src/main/java/com/conveyal/gtfs/error/MissingKeyError.java diff --git a/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java b/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java index 9630449f4..50dca9c59 100644 --- a/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java +++ b/src/main/java/com/conveyal/gtfs/error/DuplicateKeyError.java @@ -8,8 +8,8 @@ public class DuplicateKeyError extends GTFSError implements Serializable { public static final long serialVersionUID = 1L; - public DuplicateKeyError(String file, long line, String field) { - super(file, line, field); + public DuplicateKeyError(String file, long line, String field, String id) { + super(file, line, field, id); } @Override public String getMessage() { diff --git a/src/main/java/com/conveyal/gtfs/error/MissingKeyError.java b/src/main/java/com/conveyal/gtfs/error/MissingKeyError.java new file mode 100644 index 000000000..5a1ccdaeb --- /dev/null +++ b/src/main/java/com/conveyal/gtfs/error/MissingKeyError.java @@ -0,0 +1,22 @@ +package com.conveyal.gtfs.error; + +import com.conveyal.gtfs.validator.model.Priority; + +import java.io.Serializable; + +/** Indicates that a GTFS entity was not added to a table because it had no primary key. */ +public class MissingKeyError extends GTFSError implements Serializable { + public static final long serialVersionUID = 1L; + + public MissingKeyError(String file, long line, String field) { + super(file, line, field); + } + + @Override public String getMessage() { + return "Missing primary key."; + } + + @Override public Priority getPriority() { + return Priority.MEDIUM; + } +} diff --git a/src/main/java/com/conveyal/gtfs/model/Agency.java b/src/main/java/com/conveyal/gtfs/model/Agency.java index a2e301329..fd1e6031c 100644 --- a/src/main/java/com/conveyal/gtfs/model/Agency.java +++ b/src/main/java/com/conveyal/gtfs/model/Agency.java @@ -20,6 +20,11 @@ public class Agency extends Entity { public URL agency_branding_url; public String feed_id; + @Override + public String getId() { + return agency_id; + } + public static class Loader extends Entity.Loader { public Loader(GTFSFeed feed) { @@ -45,11 +50,11 @@ public void loadOneRow() throws IOException { a.agency_fare_url = getUrlField("agency_fare_url", false); a.agency_branding_url = getUrlField("agency_branding_url", false); a.feed_id = feed.feedId; - - // TODO clooge due to not being able to have null keys in mapdb - if (a.agency_id == null) a.agency_id = "NONE"; - - feed.agency.put(a.agency_id, a); + // Kludge because mapdb does not support null keys + if (a.agency_id == null) { + a.agency_id = "NONE"; + } + insertCheckingDuplicateKey(feed.agency, a, "agency_id"); } } diff --git a/src/main/java/com/conveyal/gtfs/model/Calendar.java b/src/main/java/com/conveyal/gtfs/model/Calendar.java index e0f0be87f..8ac72a000 100644 --- a/src/main/java/com/conveyal/gtfs/model/Calendar.java +++ b/src/main/java/com/conveyal/gtfs/model/Calendar.java @@ -52,7 +52,7 @@ public void loadOneRow() throws IOException { String service_id = getStringField("service_id", true); // TODO service_id can reference either calendar or calendar_dates. Service service = services.computeIfAbsent(service_id, Service::new); if (service.calendar != null) { - feed.errors.add(new DuplicateKeyError(tableName, row, "service_id")); + feed.errors.add(new DuplicateKeyError(tableName, row, "service_id", service_id)); } else { Calendar c = new Calendar(); c.sourceFileLine = row + 1; // offset line number by 1 to account for 0-based row index diff --git a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java index 081f4c8b3..db4a1e9f3 100644 --- a/src/main/java/com/conveyal/gtfs/model/CalendarDate.java +++ b/src/main/java/com/conveyal/gtfs/model/CalendarDate.java @@ -52,7 +52,8 @@ public void loadOneRow() throws IOException { Service service = services.computeIfAbsent(service_id, Service::new); LocalDate date = getDateField("date", true); if (service.calendar_dates.containsKey(date)) { - feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)")); + String keyString = String.format("(%s,%s)", service_id, date.toString()); + feed.errors.add(new DuplicateKeyError(tableName, row, "(service_id, date)", keyString)); } else { CalendarDate cd = new CalendarDate(); cd.sourceFileLine = row + 1; // offset line number by 1 to account for 0-based row index diff --git a/src/main/java/com/conveyal/gtfs/model/Entity.java b/src/main/java/com/conveyal/gtfs/model/Entity.java index c445f2a41..215d08072 100644 --- a/src/main/java/com/conveyal/gtfs/model/Entity.java +++ b/src/main/java/com/conveyal/gtfs/model/Entity.java @@ -1,6 +1,8 @@ package com.conveyal.gtfs.model; import com.beust.jcommander.internal.Sets; +import com.conveyal.gtfs.error.DuplicateKeyError; +import com.conveyal.gtfs.error.MissingKeyError; import com.conveyal.r5.analyst.progress.ProgressInputStream; import com.conveyal.gtfs.GTFSFeed; import com.conveyal.gtfs.error.DateParseError; @@ -57,8 +59,10 @@ public abstract class Entity implements Serializable { * with a sequence number. For example stop_times and shapes have no single field that uniquely * identifies a row, and the stop_sequence or shape_pt_sequence must also be considered. */ - public String getId () { - return null; + public String getId() { + // Several entities have compound keys which are handled as tuple objects, not strings. + // Fail fast if anything tries to fetch a string ID for those Entity types. + throw new UnsupportedOperationException(); } /** @@ -301,6 +305,24 @@ public void loadTable (ZipFile zip) throws IOException { } } + /** + * Insert the given value into the map, checking whether a value already exists with its key. + * The entity type must override getId() for this to work. We have to pass in the name of the key field for + * error reporting purposes because although there is a method to get the ID of an Entity there is not a method + * to get the name of the field(s) it is taken from. + */ + protected void insertCheckingDuplicateKey (Map map, E value, String keyField) { + String key = value.getId(); + if (key == null) { + feed.errors.add(new MissingKeyError(tableName, value.sourceFileLine, keyField)); + return; + } + // Map returns previous value if one was already present + E previousValue = map.put(key, value); + if (previousValue != null) { + feed.errors.add(new DuplicateKeyError(tableName, value.sourceFileLine, keyField, key)); + } + } } /** diff --git a/src/main/java/com/conveyal/gtfs/model/FareAttribute.java b/src/main/java/com/conveyal/gtfs/model/FareAttribute.java index 35afcbc97..9ea58bede 100644 --- a/src/main/java/com/conveyal/gtfs/model/FareAttribute.java +++ b/src/main/java/com/conveyal/gtfs/model/FareAttribute.java @@ -39,7 +39,7 @@ public void loadOneRow() throws IOException { String fareId = getStringField("fare_id", true); Fare fare = fares.computeIfAbsent(fareId, Fare::new); if (fare.fare_attribute != null) { - feed.errors.add(new DuplicateKeyError(tableName, row, "fare_id")); + feed.errors.add(new DuplicateKeyError(tableName, row, "fare_id", fareId)); } else { FareAttribute fa = new FareAttribute(); fa.sourceFileLine = row + 1; // offset line number by 1 to account for 0-based row index diff --git a/src/main/java/com/conveyal/gtfs/model/Route.java b/src/main/java/com/conveyal/gtfs/model/Route.java index cb9c208b8..f4965861b 100644 --- a/src/main/java/com/conveyal/gtfs/model/Route.java +++ b/src/main/java/com/conveyal/gtfs/model/Route.java @@ -32,6 +32,11 @@ public class Route extends Entity { // implements Entity.Factory public URL route_branding_url; public String feed_id; + @Override + public String getId() { + return route_id; + } + public static class Loader extends Entity.Loader { public Loader(GTFSFeed feed) { @@ -72,7 +77,7 @@ public void loadOneRow() throws IOException { r.route_text_color = getStringField("route_text_color", false); r.route_branding_url = getUrlField("route_branding_url", false); r.feed_id = feed.feedId; - feed.routes.put(r.route_id, r); + insertCheckingDuplicateKey(feed.routes, r, "route_id"); } } diff --git a/src/main/java/com/conveyal/gtfs/model/Stop.java b/src/main/java/com/conveyal/gtfs/model/Stop.java index 580030bef..26a5fabb3 100644 --- a/src/main/java/com/conveyal/gtfs/model/Stop.java +++ b/src/main/java/com/conveyal/gtfs/model/Stop.java @@ -1,10 +1,12 @@ package com.conveyal.gtfs.model; import com.conveyal.gtfs.GTFSFeed; +import com.conveyal.gtfs.error.DuplicateKeyError; import java.io.IOException; import java.net.URL; import java.util.Iterator; +import java.util.Map; public class Stop extends Entity { @@ -61,12 +63,13 @@ public void loadOneRow() throws IOException { s.stop_timezone = getStringField("stop_timezone", false); s.wheelchair_boarding = getStringField("wheelchair_boarding", false); s.feed_id = feed.feedId; - /* TODO check ref integrity later, this table self-references via parent_station */ - feed.stops.put(s.stop_id, s); + // Referential integrity is checked later after fully loaded. Stops self-reference via parent_station. + insertCheckingDuplicateKey(feed.stops, s, "stop_id"); } } + public static class Writer extends Entity.Writer { public Writer (GTFSFeed feed) { super(feed, "stops"); diff --git a/src/main/java/com/conveyal/gtfs/model/StopTime.java b/src/main/java/com/conveyal/gtfs/model/StopTime.java index b15f7b46d..daabfec14 100644 --- a/src/main/java/com/conveyal/gtfs/model/StopTime.java +++ b/src/main/java/com/conveyal/gtfs/model/StopTime.java @@ -28,7 +28,7 @@ public class StopTime extends Entity implements Cloneable, Serializable { @Override public String getId() { - return trip_id; // Needs sequence number to be unique + return trip_id; // Concatenate with sequence number to make unique } @Override diff --git a/src/main/java/com/conveyal/gtfs/model/Trip.java b/src/main/java/com/conveyal/gtfs/model/Trip.java index 601d4d60d..d0a16380e 100644 --- a/src/main/java/com/conveyal/gtfs/model/Trip.java +++ b/src/main/java/com/conveyal/gtfs/model/Trip.java @@ -20,6 +20,11 @@ public class Trip extends Entity { public int wheelchair_accessible; public String feed_id; + @Override + public String getId() { + return trip_id; + } + public static class Loader extends Entity.Loader { public Loader(GTFSFeed feed) { @@ -47,7 +52,7 @@ public void loadOneRow() throws IOException { t.bikes_allowed = getIntField("bikes_allowed", false, 0, 2); t.wheelchair_accessible = getIntField("wheelchair_accessible", false, 0, 2); t.feed_id = feed.feedId; - feed.trips.put(t.trip_id, t); + insertCheckingDuplicateKey(feed.trips, t, "trip_id"); /* Check referential integrity without storing references. Trip cannot directly reference Services or From a7fb443cc6a3a1e5d0027ff91ebdc4bad542a5d4 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 17 Mar 2023 18:38:07 +0800 Subject: [PATCH 40/46] always checkGridSize when creating grids (#844) --- .../analysis/models/AnalysisRequest.java | 19 ------- .../java/com/conveyal/r5/analyst/Grid.java | 5 ++ .../r5/analyst/WebMercatorExtents.java | 50 +++++++++++++++++-- .../com/conveyal/r5/analyst/GridTest.java | 40 ++++++++------- .../network/GridSinglePointTaskBuilder.java | 4 ++ .../network/RandomFrequencyPhasingTests.java | 1 + 6 files changed, 78 insertions(+), 41 deletions(-) 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/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/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); From 2817bd4de2f21b10a3e6aaf1b34b34e72f5aada6 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 17 Mar 2023 19:45:27 +0800 Subject: [PATCH 41/46] correct comments about polyline encoder --- src/main/java/com/conveyal/gtfs/model/Pattern.java | 10 ++++++++-- .../conveyal/r5/util/EncodedPolylineSerializer.java | 12 +++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) 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/util/EncodedPolylineSerializer.java b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java index ee2d4ffdb..eb440fc26 100644 --- a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java +++ b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java @@ -11,9 +11,15 @@ 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 is hosted at a repo outside Maven Central which has + * become unavailable on a few occations. We should evaluate licence compatibility (LGPL) for copying it into this repo + * ("vendoring" it). */ public class EncodedPolylineSerializer extends JsonSerializer { From 92cb3a3d4857acfe5f49896e0866fe9e1a9991aa Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 17 Mar 2023 20:43:22 +0800 Subject: [PATCH 42/46] avoid hard NPE failure hiding underlying problems Skip making patterns when severe problems like referential integrity errors are present, so those errors are visible instead of just NPE. --- src/main/java/com/conveyal/gtfs/GTFSFeed.java | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFSFeed.java b/src/main/java/com/conveyal/gtfs/GTFSFeed.java index f1f380601..83236ad01 100644 --- a/src/main/java/com/conveyal/gtfs/GTFSFeed.java +++ b/src/main/java/com/conveyal/gtfs/GTFSFeed.java @@ -1,6 +1,7 @@ package com.conveyal.gtfs; import com.conveyal.gtfs.error.GTFSError; +import com.conveyal.gtfs.error.ReferentialIntegrityError; import com.conveyal.gtfs.model.Agency; import com.conveyal.gtfs.model.Calendar; import com.conveyal.gtfs.model.CalendarDate; @@ -19,6 +20,7 @@ import com.conveyal.gtfs.model.StopTime; import com.conveyal.gtfs.model.Transfer; import com.conveyal.gtfs.model.Trip; +import com.conveyal.gtfs.validator.model.Priority; import com.conveyal.gtfs.validator.service.GeoUtils; import com.conveyal.r5.analyst.progress.ProgressListener; import com.google.common.collect.HashMultimap; @@ -253,7 +255,15 @@ public void loadFromFile(ZipFile zip, String fid) throws Exception { // There are conceivably cases where the extra step of identifying and naming patterns is not necessary. // In current usage we do always need them, and performing this step during load allows enforcing subsequent // read-only access. - findPatterns(); + // Find patterns only if there are no referential integrity errors or other severe problems. Those problems + // can cause pattern finding to fail hard with null pointer exceptions, causing detailed error messages to be + // lost and hiding underlying problems from the user. If high-priority problems are present, the feed should be + // presented to the user as unuseable anyway. + if (errors.stream().anyMatch(e -> e.getPriority() == Priority.HIGH)) { + LOG.warn("Feed contains high priority errors, not finding patterns. It will be useless for routing."); + } else { + findPatterns(); + } // Prevent loading additional feeds into this MapDB. loaded = true; @@ -576,11 +586,13 @@ private void namePatterns(Collection patterns) { public LineString getStraightLineForStops(String trip_id) { CoordinateList coordinates = new CoordinateList(); - LineString ls = null; + LineString lineString = null; Trip trip = trips.get(trip_id); Iterable stopTimes; stopTimes = getOrderedStopTimesForTrip(trip.trip_id); + // lineString must remain null if there are less than two stopTimes to avoid + // an exception when creating linestring. if (Iterables.size(stopTimes) > 1) { for (StopTime stopTime : stopTimes) { Stop stop = stops.get(stopTime.stop_id); @@ -588,13 +600,9 @@ public LineString getStraightLineForStops(String trip_id) { Double lon = stop.stop_lon; coordinates.add(new Coordinate(lon, lat)); } - ls = geometryFactory.createLineString(coordinates.toCoordinateArray()); + lineString = geometryFactory.createLineString(coordinates.toCoordinateArray()); } - // set ls equal to null if there is only one stopTime to avoid an exception when creating linestring - else{ - ls = null; - } - return ls; + return lineString; } /** From 45d6bdcd82e341d44626988bb3134fe4ce7b6f6b Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 17 Mar 2023 21:08:59 +0800 Subject: [PATCH 43/46] consistently set error line number from row field this is zero-based and will not match values in the Entity objects themselves, or values produced in post-validation. Adjusting them all to be one-based is a separate task. --- src/main/java/com/conveyal/gtfs/model/Entity.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/model/Entity.java b/src/main/java/com/conveyal/gtfs/model/Entity.java index 215d08072..a2395b473 100644 --- a/src/main/java/com/conveyal/gtfs/model/Entity.java +++ b/src/main/java/com/conveyal/gtfs/model/Entity.java @@ -314,13 +314,13 @@ public void loadTable (ZipFile zip) throws IOException { protected void insertCheckingDuplicateKey (Map map, E value, String keyField) { String key = value.getId(); if (key == null) { - feed.errors.add(new MissingKeyError(tableName, value.sourceFileLine, keyField)); + feed.errors.add(new MissingKeyError(tableName, row, keyField)); return; } // Map returns previous value if one was already present E previousValue = map.put(key, value); if (previousValue != null) { - feed.errors.add(new DuplicateKeyError(tableName, value.sourceFileLine, keyField, key)); + feed.errors.add(new DuplicateKeyError(tableName, row, keyField, key)); } } } From 8545de9f2ee12f240e910d42a944c96e57459348 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Sat, 18 Mar 2023 01:25:33 +0800 Subject: [PATCH 44/46] get polyline encoder 0.2 from Conveyal Maven repo --- build.gradle | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/build.gradle b/build.gradle index 3ab0dbb0c..618a7bb88 100644 --- a/build.gradle +++ b/build.gradle @@ -125,10 +125,8 @@ repositories { // Put Open Source Geospatial before Maven Central to get JAI core, see https://stackoverflow.com/a/26993223 maven { url 'https://repo.osgeo.org/repository/release/' } mavenCentral() - // TODO review whether we really need the repositories below + // Polyline encoder 0.2 is now in Maven repo maven { url 'https://maven.conveyal.com' } - // For the polyline encoder - maven { url 'https://nexus.axiomalaska.com/nexus/content/repositories/public-releases' } } // Exclude all JUnit 4 transitive dependencies - IntelliJ bug causes it to think we're using Junit 4 instead of 5. From 9079edf191caabd4cf6b093a562bf013a79bfff1 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Sat, 18 Mar 2023 01:25:33 +0800 Subject: [PATCH 45/46] get polyline encoder 0.2 from Conveyal Maven repo --- build.gradle | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/build.gradle b/build.gradle index 3ab0dbb0c..618a7bb88 100644 --- a/build.gradle +++ b/build.gradle @@ -125,10 +125,8 @@ repositories { // Put Open Source Geospatial before Maven Central to get JAI core, see https://stackoverflow.com/a/26993223 maven { url 'https://repo.osgeo.org/repository/release/' } mavenCentral() - // TODO review whether we really need the repositories below + // Polyline encoder 0.2 is now in Maven repo maven { url 'https://maven.conveyal.com' } - // For the polyline encoder - maven { url 'https://nexus.axiomalaska.com/nexus/content/repositories/public-releases' } } // Exclude all JUnit 4 transitive dependencies - IntelliJ bug causes it to think we're using Junit 4 instead of 5. From c8adfeca7ce80454bf3da4313318447f31bae38e Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Tue, 21 Mar 2023 09:11:08 +0800 Subject: [PATCH 46/46] Update javadoc about dependency --- .../java/com/conveyal/r5/util/EncodedPolylineSerializer.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java index eb440fc26..ee5e8ba35 100644 --- a/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java +++ b/src/main/java/com/conveyal/r5/util/EncodedPolylineSerializer.java @@ -17,9 +17,8 @@ * 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 is hosted at a repo outside Maven Central which has - * become unavailable on a few occations. We should evaluate licence compatibility (LGPL) for copying it into this repo - * ("vendoring" it). + * 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 {