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

Conversation

ansoncfit
Copy link
Member

@ansoncfit ansoncfit commented Jun 13, 2023

Addresses #714: with frequency-based routes, users are often distracted by spurious changes in access from random schedule differences between scenarios.

This PR allows "locking" schedules by setting the seed used for randomizing frequency offsets to a deterministic value for each origin (namely, a multiple of fromLat, as is done in the multi-criteria router: see FrequencyRandomOffsets). It also adds a field to the analysis request to activate this mode (see AnalysisRequest). With lockSchedules: true, a given origin will use the same frequency offsets across scenarios, so resulting changes in access should be systematic ones. This PR also includes other minor changes for related tests and documentation.

To test, fetch isochrones repeatedly along an infrequent frequency-based route. Without lockSchedules, the isochrone should "waver." With this PR and lockSchedules: true (or using build v6.9-7-ged8378e, which hardcodes the logic to avoid needing to start a backend server via https://github.com/conveyal/r5/tree/lock-lock-schedules), there should be no variability with repeatedly fetched isochrones.

In documentation, we should encourage users to run analysis without locking schedules first, to get a sense of variability. If certain corridors show large variability, they may want to use phasing. We've discussed other longer-term changes related to optimized schedules, but the approach proposed here is a minimally invasive one that addresses multiple user requests.

@ansoncfit ansoncfit requested a review from abyrd June 13, 2023 16:58

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.

@abyrd
Copy link
Member

abyrd commented Aug 4, 2023

In light of recent discussions summarized at #714 (comment) do we still want to add a lockSchedules parameter or just use a custom/experimental worker build if this is ever needed? Have we been able to handle most cases where this was an issue by simply switching to exact-times or phased schedules, or increasing the number of MC draws?

@ansoncfit ansoncfit marked this pull request as draft October 27, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants