From e7796b3e4d56a983b90e4b05e95d0da37450eb42 Mon Sep 17 00:00:00 2001 From: abyrd Date: Wed, 22 Nov 2023 11:38:11 +0800 Subject: [PATCH 1/6] change CSV headers and column order, add comments --- .../TemporalDensityCsvResultWriter.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java b/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java index 61279629c..0e86fa989 100644 --- a/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java +++ b/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java @@ -31,15 +31,15 @@ public CsvResultType resultType () { public String[] columnHeaders () { List headers = new ArrayList<>(); // The ids of the freeform origin point and destination set - headers.add("originId"); - headers.add("destId"); + headers.add("origin"); + headers.add("destinations"); headers.add("percentile"); + // The number of minutes needed to reach d destination opportunities + headers.add("D" + dualThreshold); + // The opportunity density during each of 120 minutes for (int m = 0; m < 120; m += 1) { - // The opportunity density over travel minute m headers.add(Integer.toString(m)); } - // The number of minutes needed to reach d destination opportunities - headers.add("D" + dualThreshold); return headers.toArray(new String[0]); } @@ -67,20 +67,24 @@ public Iterable rowValues (RegionalWorkResult workResult) { List row = new ArrayList<>(125); row.add(originId); row.add(task.destinationPointSetKeys[d]); - row.add(Integer.toString(p)); - // One density value for each of 120 minutes + row.add(Integer.toString(task.percentiles[p])); + // One column containing dual accessibility value double[] densitiesPerMinute = percentilesForDestPointset[p]; - for (int m = 0; m < 120; m++) { - row.add(Double.toString(densitiesPerMinute[m])); - } - // One dual accessibility value int m = 0; double sum = 0; + // Find smallest integer M such that we have already reached D destinations after M minutes of travel. while (sum < dualThreshold && m < 120) { sum += densitiesPerMinute[m]; m += 1; } + // -1 indicates the threshold number of opportunities had still not been reached after the highest + // travel time cutoff specified in the analysis. row.add(Integer.toString(m >= 120 ? -1 : m)); + // One density value for each of 120 one-minute bins. + // Column labeled 10 contains the number of opportunities reached after 10 to 11 minutes of travel. + for (m = 0; m < 120; m++) { + row.add(Double.toString(densitiesPerMinute[m])); + } rows.add(row.toArray(new String[row.size()])); } } From 46abd8a92a2656f1704b6e27490a27afae9a5cfd Mon Sep 17 00:00:00 2001 From: abyrd Date: Wed, 22 Nov 2023 13:09:11 +0800 Subject: [PATCH 2/6] do not use "final" int before it's initialized --- .../analysis/results/TemporalDensityCsvResultWriter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java b/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java index 0e86fa989..02a6168ad 100644 --- a/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java +++ b/src/main/java/com/conveyal/analysis/results/TemporalDensityCsvResultWriter.java @@ -35,7 +35,7 @@ public String[] columnHeaders () { headers.add("destinations"); headers.add("percentile"); // The number of minutes needed to reach d destination opportunities - headers.add("D" + dualThreshold); + headers.add("dual"); // The opportunity density during each of 120 minutes for (int m = 0; m < 120; m += 1) { headers.add(Integer.toString(m)); From 1bf792f4654d78ab189dc0ac91d79ad8335f2081 Mon Sep 17 00:00:00 2001 From: abyrd Date: Wed, 22 Nov 2023 18:40:00 +0800 Subject: [PATCH 3/6] add shadow jar configuration back to gradle build This started working again, so for consistency with past releases I built and attached a shadow jar to the v7.0 release on github. We may want to eventually remove this though: we do not use it in our workflow but do use the zip distribution and gradle runBackend. --- build.gradle | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/build.gradle b/build.gradle index 7bc3a4fff..945b4a434 100644 --- a/build.gradle +++ b/build.gradle @@ -3,11 +3,12 @@ plugins { id 'application' id 'maven-publish' id 'com.palantir.git-version' version '2.0.0' + id 'com.github.johnrengelman.shadow' version '8.1.1' } group = 'com.conveyal' // set version to `git describe --tags --always --first-parent`, plus '.dirty' if local changes are present. -version gitVersion() +version 'v7.0' //gitVersion() java { toolchain { @@ -19,7 +20,7 @@ jar { // For Java 11+ Modules, specify a module name. // Do not create module-info.java until all our dependencies specify a module name. // Main-Class BackendMain will start a local backend. - // Build-Jdk-Spec mimics a Maven manifest entry that helps us automatically install the right JVM. + // Build-Jdk-Spec mimics a Maven manifest entry that helps us automatically install or select the right JVM. // Implementation-X attributes are needed for ImageIO (used by Geotools) to initialize in some environments. manifest { attributes 'Automatic-Module-Name': 'com.conveyal.r5', @@ -31,6 +32,10 @@ jar { } } +shadowJar { + mergeServiceFiles() +} + // Allow reflective access by ObjectDiffer to normally closed Java internals. Used for round-trip testing serialization. // IntelliJ seems not to pass these JVM arguments when running tests from within the IDE, so the Kryo serialization // tests may only succeed under command line Gradle. @@ -42,8 +47,8 @@ test { '--add-opens=java.base/java.lang=ALL-UNNAMED'] } -// `gradle publish` will upload both shadow and simple JAR to Github Packages -// On GH Actions, GITHUB_ACTOR env variable is supplied without specifying it in action yml. +// Set up publication of jar files to GitHub Packages Maven repository. +// On GitHub Actions, GITHUB_ACTOR env variable is supplied without specifying it in action yml. publishing { repositories { maven { @@ -56,10 +61,12 @@ publishing { } } publications { - // The presence of the shadow plugin somehow causes the shadow-jar to also be automatically included in this - // publication. Ideally we want to produce the shadow jar and upload it to S3 as a worker, but only publish the - // much smaller plain JAR without dependencies to Github Packages. On the other hand, we may want to publish - // shadow jars for tagged releases. + // The shadow plugin automatically creates and registers a component called "shadow" for integration with this + // Maven publish plugin. `gradle publish` will then upload both shadow jar and simple jar to Github Packages. + // See https://imperceptiblethoughts.com/shadow/getting-started/#default-java-groovy-tasks + // To run R5 with dependencies, Conveyal does not use shadow jars anymore, only the zip distribution or runBackend. + // For development builds and tests we don't need to produce a shadow jar, only publish the much smaller plain + // jar without dependencies to Github Packages. For now, we continue to attach shadow jars to tagged releases. gpr(MavenPublication) { from(components.java) } From 6b41035eb19fd121fc55193b763bc858e953950c Mon Sep 17 00:00:00 2001 From: abyrd Date: Wed, 22 Nov 2023 18:44:50 +0800 Subject: [PATCH 4/6] remove hard-coded release version --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 945b4a434..d93f0e90b 100644 --- a/build.gradle +++ b/build.gradle @@ -8,7 +8,7 @@ plugins { group = 'com.conveyal' // set version to `git describe --tags --always --first-parent`, plus '.dirty' if local changes are present. -version 'v7.0' //gitVersion() +version gitVersion() java { toolchain { From 484b53df003c5bd910c38a7926741b1b9609866c Mon Sep 17 00:00:00 2001 From: abyrd Date: Wed, 22 Nov 2023 19:17:58 +0800 Subject: [PATCH 5/6] add comment explaining reason for recent patch --- .../com/conveyal/analysis/results/CsvResultWriter.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/analysis/results/CsvResultWriter.java b/src/main/java/com/conveyal/analysis/results/CsvResultWriter.java index 018931209..ca0cf09c5 100644 --- a/src/main/java/com/conveyal/analysis/results/CsvResultWriter.java +++ b/src/main/java/com/conveyal/analysis/results/CsvResultWriter.java @@ -41,7 +41,12 @@ public abstract class CsvResultWriter extends BaseResultWriter implements Region */ public abstract CsvResultType resultType (); - /** Override to provide column names for this CSV writer. */ + /** + * Override to provide column names for this CSV writer. + * NOTE: Due to Java weirdness, subclass implementations of this method will be called by the CsvResultWriter + * constructor at a time when fields of the subclass remain initialized, but uninitialized final primitive + * fields are still readable! Do not read subclass fields in these implementations until/unless this is restructured. + */ protected abstract String[] columnHeaders (); /** Override to extract row values from a single origin result. */ From 1b322b4e3f5d9f0c8473a2922c9270ea30e65b95 Mon Sep 17 00:00:00 2001 From: abyrd Date: Fri, 24 Nov 2023 16:37:18 +0800 Subject: [PATCH 6/6] fix comment on NETWORK_FORMAT_VERSION --- .../com/conveyal/r5/kryo/KryoNetworkSerializer.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/conveyal/r5/kryo/KryoNetworkSerializer.java b/src/main/java/com/conveyal/r5/kryo/KryoNetworkSerializer.java index 0d671f29f..7180074f4 100644 --- a/src/main/java/com/conveyal/r5/kryo/KryoNetworkSerializer.java +++ b/src/main/java/com/conveyal/r5/kryo/KryoNetworkSerializer.java @@ -45,11 +45,13 @@ public abstract class KryoNetworkSerializer { * the serialization format itself does not change. This will ensure newer workers will not load cached older files. * We considered using an ISO date string as the version but that could get confusing when seen in filenames. * - * History of Network Version (NV) changes: - * nv4 2023-11-02 WebMercatorGridPointSet now contains nested WebMercatorExtents - * nv3 2023-01-18 use Kryo 5 serialization format - * nv2 2022-04-05 - * nv1 2021-04-30 stopped using r5 version string (which caused networks to be rebuilt for every new r5 version) + * History of Network Version (NV) changes (in production releases): + * nv3 since v7.0: switched to Kryo 5 serialization, WebMercatorGridPointSet now contains nested WebMercatorExtents + * nv2 since 2022-04-05 + * nv1 since 2021-04-30: stopped rebuilding networks for every new r5 version, manually setting this version string + * + * When prototyping new features, use a unique identifier such as the branch or a commit ID, not sequential nvX ones. + * This avoids conflicts when multiple changes are combined in a single production release, or some are abandoned. */ public static final String NETWORK_FORMAT_VERSION = "nv3";