Skip to content

Commit

Permalink
Refactor: Remove deprecated / unused regional analysis fields
Browse files Browse the repository at this point in the history 
Remove old fields that are no longer used. Should be accompanied by a MongoDB migration to clean up old regional analyses.

We recently removed a similar worker version related flag from the UI here: conveyal/ui#2064
  • Loading branch information
@trevorgerhardt
trevorgerhardt committed Feb 18, 2025
1 parent 6f35697 commit 2837d10
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 103 deletions.
90 changes: 25 additions & 65 deletions src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URI;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
Expand Down Expand Up @@ -399,53 +398,36 @@ private UrlWithHumanName getRegionalResults (Request req, Response res) throws I
final UserPermissions userPermissions = UserPermissions.from(req);
RegionalAnalysis analysis = getAnalysis(regionalAnalysisId, userPermissions);

// Which channel to extract from results with multiple values per origin (for different travel time cutoffs)
// and multiple output files per analysis (for different percentiles of travel time and/or different
// destination pointsets). These initial values are for older regional analysis results with only a single
// cutoff, and no percentile or destination gridId in the file name.
// For newer analyses that have multiple cutoffs, percentiles, or destination pointsets, these initial values
// are coming from deprecated fields, are not meaningful and will be overwritten below from query parameters.
int percentile = analysis.travelTimePercentile;
int cutoffMinutes = analysis.cutoffMinutes;
String destinationPointSetId = analysis.grid;

// Handle newer regional analyses with multiple cutoffs in an array.
// If a query parameter is supplied, range check it, otherwise use the middle value in the list.
// The cutoff variable holds the actual cutoff in minutes, not the position in the array of cutoffs.
if (analysis.cutoffsMinutes != null) {
int nCutoffs = analysis.cutoffsMinutes.length;
checkState(nCutoffs > 0, "Regional analysis has no cutoffs.");
cutoffMinutes = getIntQueryParameter(req, "cutoff", analysis.cutoffsMinutes[nCutoffs / 2]);
checkArgument(new TIntArrayList(analysis.cutoffsMinutes).contains(cutoffMinutes),
"Travel time cutoff for this regional analysis must be taken from this list: (%s)",
Ints.join(", ", analysis.cutoffsMinutes)
);
}
int nCutoffs = analysis.cutoffsMinutes.length;
checkState(nCutoffs > 0, "Regional analysis has no cutoffs.");
int cutoffMinutes = getIntQueryParameter(req, "cutoff", analysis.cutoffsMinutes[nCutoffs / 2]);
checkArgument(new TIntArrayList(analysis.cutoffsMinutes).contains(cutoffMinutes),
"Travel time cutoff for this regional analysis must be taken from this list: (%s)",
Ints.join(", ", analysis.cutoffsMinutes)
);

// Handle newer regional analyses with multiple percentiles in an array.
// If a query parameter is supplied, range check it, otherwise use the middle value in the list.
// The percentile variable holds the actual percentile (25, 50, 95) not the position in the array.
if (analysis.travelTimePercentiles != null) {
int nPercentiles = analysis.travelTimePercentiles.length;
checkState(nPercentiles > 0, "Regional analysis has no percentiles.");
percentile = getIntQueryParameter(req, "percentile", analysis.travelTimePercentiles[nPercentiles / 2]);
checkArgument(new TIntArrayList(analysis.travelTimePercentiles).contains(percentile),
"Percentile for this regional analysis must be taken from this list: (%s)",
Ints.join(", ", analysis.travelTimePercentiles));
int nPercentiles = analysis.travelTimePercentiles.length;
checkState(nPercentiles > 0, "Regional analysis has no percentiles.");
int percentile = getIntQueryParameter(req, "percentile", analysis.travelTimePercentiles[nPercentiles / 2]);
checkArgument(new TIntArrayList(analysis.travelTimePercentiles).contains(percentile),
"Percentile for this regional analysis must be taken from this list: (%s)",
Ints.join(", ", analysis.travelTimePercentiles));

// Handle regional analyses with multiple destination pointsets per analysis.
int nGrids = analysis.destinationPointSetIds.length;
checkState(nGrids > 0, "Regional analysis has no grids.");
String destinationPointSetId = req.queryParams("destinationPointSetId");
if (destinationPointSetId == null) {
destinationPointSetId = analysis.destinationPointSetIds[0];
}
checkArgument(Arrays.asList(analysis.destinationPointSetIds).contains(destinationPointSetId),
"Destination gridId must be one of: %s",
String.join(",", analysis.destinationPointSetIds));

// Handle even newer regional analyses with multiple destination pointsets per analysis.
if (analysis.destinationPointSetIds != null) {
int nGrids = analysis.destinationPointSetIds.length;
checkState(nGrids > 0, "Regional analysis has no grids.");
destinationPointSetId = req.queryParams("destinationPointSetId");
if (destinationPointSetId == null) {
destinationPointSetId = analysis.destinationPointSetIds[0];
}
checkArgument(Arrays.asList(analysis.destinationPointSetIds).contains(destinationPointSetId),
"Destination gridId must be one of: %s",
String.join(",", analysis.destinationPointSetIds));
}
// We started implementing the ability to retrieve and display partially completed analyses.
// We eventually decided these should not be available here at the same endpoint as complete, immutable results.
if (broker.findJob(regionalAnalysisId) != null) {
Expand Down Expand Up @@ -536,10 +518,7 @@ private RegionalAnalysis createRegionalAnalysis (Request req, Response res) thro
opportunityDatasets.add(opportunityDataset);
task.destinationPointSetKeys[i] = opportunityDataset.storageLocation();
}
// For backward compatibility with old workers, communicate any single pointSet via the deprecated field.
if (nPointSets == 1) {
task.grid = task.destinationPointSetKeys[0];
}

// Check that we have either a single freeform pointset, or only gridded pointsets at indentical zooms.
// The worker will perform equivalent checks via the GridTransformWrapper constructor,
// WebMercatorExtents.expandToInclude and WebMercatorExtents.forPointsets. Potential to share code.
Expand Down Expand Up @@ -624,34 +603,15 @@ private RegionalAnalysis createRegionalAnalysis (Request req, Response res) thro
regionalAnalysis.workerVersion = analysisRequest.workerVersion;
regionalAnalysis.zoom = task.zoom;

// Store the full array of multiple cutoffs which will be understood by newer workers and backends,
// rather then the older single cutoff value.
// Store the full array of multiple cutoffs.
checkNotNull(analysisRequest.cutoffsMinutes);
checkArgument(analysisRequest.cutoffsMinutes.length > 0);
regionalAnalysis.cutoffsMinutes = analysisRequest.cutoffsMinutes;
if (analysisRequest.cutoffsMinutes.length == 1) {
// Ensure older workers expecting a single cutoff will still function.
regionalAnalysis.cutoffMinutes = analysisRequest.cutoffsMinutes[0];
} else {
// Store invalid value in deprecated field (-1 was already used) to make it clear it should not be used.
regionalAnalysis.cutoffMinutes = -2;
}

// Same process as for cutoffsMinutes, but for percentiles.
checkNotNull(analysisRequest.percentiles);
checkArgument(analysisRequest.percentiles.length > 0);
regionalAnalysis.travelTimePercentiles = analysisRequest.percentiles;
if (analysisRequest.percentiles.length == 1) {
regionalAnalysis.travelTimePercentile = analysisRequest.percentiles[0];
} else {
regionalAnalysis.travelTimePercentile = -2;
}

// Propagate any changes to the cutoff and percentiles arrays down into the nested RegionalTask.
// TODO propagate single (non-array) values for old workers
// TODO propagate the other direction, setting these values when initializing the task
task.cutoffsMinutes = regionalAnalysis.cutoffsMinutes;
task.percentiles = regionalAnalysis.travelTimePercentiles;

// Persist this newly created RegionalAnalysis to Mongo.
// This assigns it creation/update time stamps and an ID, which is needed to name any output CSV files.
Expand Down
21 changes: 0 additions & 21 deletions src/main/java/com/conveyal/analysis/models/RegionalAnalysis.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ public class RegionalAnalysis extends Model implements Cloneable {
public String projectId;
public String scenarioId;

public int variant;

public String workerVersion;

public int zoom;
Expand All @@ -37,33 +35,14 @@ public class RegionalAnalysis extends Model implements Cloneable {
*/
public RegionalTask request;

/**
* Single percentile of travel time being used in this analysis. Older analyses could have only one percentile.
* If the analysis is pre-percentiles and is using Andrew Owen-style accessibility, value is -1.
* If the analysis has more than one percentile, value is -2.
*/
@Deprecated
public int travelTimePercentile = -1;

/**
* Newer regional analyses (since release X in February 2020) can have more than one percentile.
* If this is non-null it completely supersedes travelTimePercentile, which should be ignored.
*/
public int[] travelTimePercentiles;

/** Single destination pointset id (for older analyses that did not allow multiple sets of destinations). */
@Deprecated
public String grid;

public String[] destinationPointSetIds;

/**
* Older analyses (up to about January 2020, before release X) had only one cutoff.
* New analyses with more than one cutoff will have this set to -2.
*/
@Deprecated
public int cutoffMinutes;

/**
* The different travel time thresholds used in this analysis to include or exclude opportunities from an
* accessibility metric. Supersedes the singular cutoffMinutes used in older analyses. For non-step decay functions
Expand Down
17 changes: 0 additions & 17 deletions src/main/java/com/conveyal/r5/analyst/cluster/RegionalTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,6 @@
*/
public class RegionalTask extends AnalysisWorkerTask implements Cloneable {

/**
* The storage key for the pointset we will compute access to (e.g. regionId/datasetId.grid).
* This is named grid instead of destinationPointSetId for backward compatibility, namely the ability to start
* regional jobs on old worker versions that expect the property "grid".
*
* Overloaded to specify a set of destination points which may or may not have densities attached.
* In fact this ID is taken from a field called "opportunityDatasetId" in the request coming from the UI. So we've
* got several slightly conflicting names and concepts.
*
* TODO revise and improve the below explanation:
* If this is not blank, the default TravelTimeSurfaceTask will be overridden; returnInVehicleTimes,
* returnWaitTimes, and returnPaths will be set to false; and the returned results will be an accessibility value
* per origin, rather than a grid of travel times from that origin.
*/
@Deprecated
public String grid;

/**
* Key for pointset (e.g. regionId/datasetId.pointset) from which to calculate travel times or accessibility
*/
Expand Down

0 comments on commit 2837d10

Please sign in to comment.