Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting seed for frequency offsets #881

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class AnalysisRequest {
public float toLon;
public int fromTime;
public int monteCarloDraws = 200;
public boolean lockSchedules = false;
public int toTime;
public String transitModes;
public float walkSpeed;
Expand Down Expand Up @@ -244,6 +245,7 @@ public void populateTask (AnalysisWorkerTask task, UserPermissions userPermissio
task.cutoffsMinutes = cutoffsMinutes;

task.logRequest = logRequest;
task.lockSchedules = lockSchedules;

task.accessModes = getEnumSetFromString(accessModes);
task.directModes = getEnumSetFromString(directModes);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public FastRaptorWorker (TransitLayer transitLayer, AnalysisWorkerTask request,
this.accessStops = accessStops;
this.servicesActive = transit.getActiveServicesForDate(request.date);

offsets = new FrequencyRandomOffsets(transitLayer);
offsets = new FrequencyRandomOffsets(transitLayer, request);

// compute number of minutes for scheduled search
nMinutes = request.getTimeWindowLengthMinutes();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.conveyal.r5.profile;

import com.conveyal.analysis.models.AnalysisRequest;
import com.conveyal.r5.transit.TransitLayer;
import com.conveyal.r5.transit.TripPattern;
import com.conveyal.r5.transit.TripSchedule;
Expand All @@ -12,8 +13,6 @@
import java.util.HashMap;
import java.util.Map;

import static com.google.common.base.Preconditions.checkElementIndex;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;

/**
Expand Down Expand Up @@ -44,13 +43,19 @@ public class FrequencyRandomOffsets {
private final Map<TripSchedule, int[]> offsetsForTripSchedule = new HashMap<>();

/** The mersenne twister is a higher quality random number generator than the one included with Java */
private MersenneTwister mt = new MersenneTwister();
private MersenneTwister mt;

public FrequencyRandomOffsets(TransitLayer data) {
public FrequencyRandomOffsets(TransitLayer data, ProfileRequest request) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the entire request necessary to be passed as a parameter? I would recommend passing either the seed itself or a MersenneTwister.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we want to allow locking the schedules at all I agree: it would be better to pass in only the seed to deter further coupling with the ProfileRequest. The seed should probably include both lat and lon to avoid correlation between all origins in a row across a grid.

I don't see a clear justification for changing the schedules from one origin to another, but locking them at the per-origin (or per-row) level. This would lead to apparently stable differences between neighboring origins that use the same routes to reach most destinations, which are likely to be misinterpreted.

The effects of knowingly deriving results from one single permutation of the schedules are clearer if that permutation is the same for all origins. In that case the seed could just be derived from the TransitLayer (e.g. its center point), and the parameter can just be a boolean for whether to do this.

this.data = data;
if (!data.hasFrequencies) {
return;
}
if (request == null || !request.lockSchedules) {
this.mt = new MersenneTwister();
} else {
// Use a fixed seed for each origin
this.mt = new MersenneTwister((int) (request.fromLat * 1e9));
}
// Create skeleton empty data structure with slots for all offsets that will be generated.
for (int pattIdx = 0; pattIdx < data.tripPatterns.size(); pattIdx++) {
TripPattern tp = data.tripPatterns.get(pattIdx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public McRaptorSuboptimalPathProfileRouter (TransportNetwork network, ProfileReq
this.touchedPatterns = new BitSet(network.transitLayer.tripPatterns.size());
this.patternsNearDestination = new BitSet(network.transitLayer.tripPatterns.size());
this.servicesActive = network.transitLayer.getActiveServicesForDate(req.date);
this.offsets = new FrequencyRandomOffsets(network.transitLayer);
this.offsets = new FrequencyRandomOffsets(network.transitLayer, request);
this.saveFinalStates = saveFinalStates;
if (saveFinalStates) this.finalStatesByDepartureTime = new TIntObjectHashMap<>();

Expand Down Expand Up @@ -150,7 +150,8 @@ public Collection<McRaptorState> route () {

// We start at end of time window and work backwards (which is what range-RAPTOR does, in case we
// re-implement that here). We use a constrained random walk to choose which departure minutes to sample as we
// work backward through the time window. According to others (Owen and Jiang?), this is a good way to reduce
// work backward through the time window. According to Owen and Murphy (2019)
// https://www.jtlu.org/index.php/jtlu/article/view/1379), this is a good way to reduce
// the number of samples without causing an issue with variance in results. This value is the constraint
// (upper limit) on the walk.
// multiply by two because E[random] = 1/2 * max.
Expand Down Expand Up @@ -535,7 +536,7 @@ private Collection<McRaptorState> doPropagationToDestination(int departureTime)
}

private ArrayList<Integer> generateDepartureTimesToSample (ProfileRequest request) {
// See Owen and Jiang 2016 (unfortunately no longer available online), add between f / 2 and
// See Owen and Murphy 2019 (https://www.jtlu.org/index.php/jtlu/article/view/1379), add between f / 2 and
// f + f / 2, where f is the mean step.
int randomWalkStepMean = (request.toTime - request.fromTime) / request.monteCarloDraws;
int randomWalkStepWidthOneSided = randomWalkStepMean / 2;
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/conveyal/r5/profile/ProfileRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ public class ProfileRequest implements Serializable, Cloneable {

/** the maximum number of options presented PER ACCESS MODE */
public int limit;

public boolean lockSchedules = false;

/** The modes used to access transit */
@JsonSerialize(using = LegModeSetSerializer.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void testPhasing () {
layer.tripPatterns.add(pattern2);
layer.rebuildTransientIndexes();

FrequencyRandomOffsets fro = new FrequencyRandomOffsets(layer);
FrequencyRandomOffsets fro = new FrequencyRandomOffsets(layer, null);
fro.randomize();

// check that phasing is correct
Expand Down Expand Up @@ -136,7 +136,7 @@ public void testPhasingAtLastStop () {
layer.tripPatterns.add(pattern2);
layer.rebuildTransientIndexes();

FrequencyRandomOffsets fro = new FrequencyRandomOffsets(layer);
FrequencyRandomOffsets fro = new FrequencyRandomOffsets(layer, null);
fro.randomize();

// check that phasing is correct
Expand Down