Skip to content

Fix frequency bounds checking - #2540

Merged
abyrd merged 5 commits into
opentripplanner:dev-1.xfrom
thatsmydoing:frequencybounds
Jul 30, 2019
Merged

abyrd merged 5 commits into
opentripplanner:dev-1.xfrom
thatsmydoing:frequencybounds

Conversation

@thatsmydoing

Copy link
Copy Markdown
Contributor

Previously, stop offsets were not taken into account, so you could not ride on a trip past it's frequency end time. This causes issues when you have stops A (00:00:00), B (00:30:00), C (01:00:00). If the end_time is at 11:45, a trip from A-B of 11:30-12:00 is valid, but a trip from B-C of 12:00-12:30 should be valid but is not.

This also fixes stop time calculation for exact trips where the (end - start) is not a multiple of headway. Previously, it calculated based on the end time, but that's a wrong assumption to make.

Tests for the fixes have also been added.

Previously, stop offsets were not taken into account, so you could not
ride on a trip past it's frequency end time, even if you could have
boarded it at an earlier stop (when it was still within the frequency
range). This fix allows that to happen.

This also fixes stop time calculation for exact trips where the (end -
start) is not a multiple of headway. Previously, it calculated based on
the end time, but that's a wrong assumption to make.

Tests for the fixes have also been added.
@abyrd

abyrd commented Mar 29, 2018

Copy link
Copy Markdown
Member

Thanks for the contribution @thatsmydoing. Could you add a few code comments to this PR clarifying the assumptions behind the code, namely that the start and end times are the time window during which departures happen from the first stop on the trip?

This does make me wonder what happens if someone searches for departures at a stop later than (freqEndTime + stopOffset - headway). While a vehicle could pass during that time window, it's unknown whether the last bus has already passed.

@thatsmydoing

Copy link
Copy Markdown
Contributor Author

There already is a comment saying that in nextDepartureTime. I can add one to prevArrivalTime too if that's what you'd like.

This mostly targets exact frequency trips since there's no ambiguity with how the schedule is supposed to work. I have an in progress patch for dealing with inexact frequency trips, but that one has some assumptions which may or may not be okay.

@abyrd

abyrd commented Mar 29, 2018

Copy link
Copy Markdown
Member

If there's already a comment, no need to add another one. I didn't hop between the diff and the original file so I missed it. This all sounds good, it's a good thing to have someone else revising this part of the code because we very rarely used it. Do you wan to add your other non-exact frequency updates to this same PR?

@thatsmydoing

Copy link
Copy Markdown
Contributor Author

Okay, I'd prefer you go ahead and merge this. The non-exact frequency patch has a bit of room for discussion so I'd like to keep that one separate.

@abyrd
abyrd changed the base branch from master to dev-1.x January 22, 2019 07:29
@abyrd
abyrd requested a review from a team January 22, 2019 07:29
@abyrd

abyrd commented Jan 22, 2019

Copy link
Copy Markdown
Member

This PR has been updated to current dev-1.x branch. Continuous Integration reports a build failure because we have replaced the One Bus Away GTFS model classes with internal OTP GTFS model classes. This will need to be fixed before we can merge this PR.

@abyrd

abyrd commented Jul 30, 2019

Copy link
Copy Markdown
Member

Merging this with a single review as a simple self-containted bug fix, since it adds a test to demonstrate the fix and I have received additional complaints about this particular bug.

@abyrd
abyrd merged commit 3e1aea3 into opentripplanner:dev-1.x Jul 30, 2019
@abyrd abyrd mentioned this pull request Jul 30, 2019
@t2gran t2gran added the X OTP1 ~ Not in use any more ~ Fix or backport to the 1.x version of OTP label Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

X OTP1 ~ Not in use any more ~ Fix or backport to the 1.x version of OTP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants