-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix timetable generation for StopTimeUpdate containing only arrival or departure #6392
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
Draft
miklcct
wants to merge
16
commits into
opentripplanner:dev-2.x
Choose a base branch
from
Jnction:arrival-only-tripupdate
base: dev-2.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+139
−19
Draft
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
863c0cd
add test for #6391
miklcct 0733694
fix departure / arrival only StopTimeUpdates causing negative dwell /…
miklcct 8aa9889
update test on the new behaviour
miklcct 1a09dce
apply review suggestion
miklcct e29e4a1
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct 942ea8e
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct f121b45
add new test
miklcct 98bbf2e
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct 9a7bf17
add new test
miklcct 961ce8f
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct a1db5e5
formatting
miklcct 84997e2
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct 111d1dd
update test
miklcct 4680292
formatting
miklcct a4799ca
Merge tag 'v2.7.0' into arrival-only-tripupdate
miklcct 0229c9f
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -312,7 +312,7 @@ public void fixIncoherentTimes() { | |
stopTimeUpdateBuilder.setStopSequence(1); | ||
stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SCHEDULED); | ||
var stopTimeEventBuilder = stopTimeUpdateBuilder.getArrivalBuilder(); | ||
stopTimeEventBuilder.setDelay(0); | ||
stopTimeEventBuilder.setDelay(900); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this test fail if you don't change it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The time will be valid after my code change because the bus can make up time during its journey. |
||
stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(1); | ||
stopTimeUpdateBuilder.setStopSequence(2); | ||
stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SCHEDULED); | ||
|
@@ -475,7 +475,8 @@ public void testUpdateWithRequiredNoDataDelayPropagationWhenItsNotRequired() { | |
assertEquals(0, updatedTripTimes.getArrivalDelay(1)); | ||
assertEquals(0, updatedTripTimes.getDepartureDelay(1)); | ||
assertEquals(-100, updatedTripTimes.getArrivalDelay(2)); | ||
assertEquals(-100, updatedTripTimes.getDepartureDelay(2)); | ||
// the stop is a timepoint so the bus will wait until the scheduled time | ||
assertEquals(0, updatedTripTimes.getDepartureDelay(2)); | ||
assertTrue(updatedTripTimes.getDepartureTime(1) < updatedTripTimes.getArrivalTime(2)); | ||
|
||
// REQUIRED_NO_DATA propagation type should always set NO_DATA flags' | ||
|
@@ -514,7 +515,8 @@ public void testUpdateWithRequiredNoDataDelayPropagationWhenItsRequired() { | |
assertEquals(-700, updatedTripTimes.getArrivalDelay(1)); | ||
assertEquals(-700, updatedTripTimes.getDepartureDelay(1)); | ||
assertEquals(-700, updatedTripTimes.getArrivalDelay(2)); | ||
assertEquals(-700, updatedTripTimes.getDepartureDelay(2)); | ||
// the stop is a timepoint so the bus will wait until the scheduled time | ||
assertEquals(0, updatedTripTimes.getDepartureDelay(2)); | ||
assertTrue(updatedTripTimes.getDepartureTime(1) < updatedTripTimes.getArrivalTime(2)); | ||
|
||
// REQUIRED_NO_DATA propagation type should always set NO_DATA flags' | ||
|
@@ -547,13 +549,17 @@ public void testUpdateWithRequiredNoDataDelayPropagationOnArrivalTime() { | |
SERVICE_DATE, | ||
BackwardsDelayPropagationType.REQUIRED_NO_DATA | ||
); | ||
// if arrival time is not defined but departure time is not and the arrival time is greater | ||
// than to departure time on a stop, we should not try to fix it by default because the spec | ||
// only allows you to drop all estimates for a stop when it's passed according to schedule | ||
assertTrue(result.isFailure()); | ||
|
||
result.ifFailure(err -> { | ||
assertEquals(err.errorType(), NEGATIVE_DWELL_TIME); | ||
// the spec does not require you to supply both arrival and departure on a StopTimeUpdate | ||
// it says: | ||
// either arrival or departure must be provided within a StopTimeUpdate - both fields cannot be | ||
// empty | ||
// therefore the processing should succeed even if only one of them is given | ||
assertTrue(result.isSuccess()); | ||
result.ifSuccess(p -> { | ||
var updatedTripTimes = p.getTripTimes(); | ||
assertNotNull(updatedTripTimes); | ||
assertEquals(15, updatedTripTimes.getArrivalDelay(2)); | ||
assertEquals(15, updatedTripTimes.getDepartureDelay(2)); | ||
}); | ||
} | ||
|
||
|
@@ -586,7 +592,8 @@ public void testUpdateWithRequiredDelayPropagationWhenItsRequired() { | |
assertEquals(-700, updatedTripTimes.getArrivalDelay(1)); | ||
assertEquals(-700, updatedTripTimes.getDepartureDelay(1)); | ||
assertEquals(-700, updatedTripTimes.getArrivalDelay(2)); | ||
assertEquals(-700, updatedTripTimes.getDepartureDelay(2)); | ||
// the stop is a timepoint so the bus will wait until the scheduled time | ||
assertEquals(0, updatedTripTimes.getDepartureDelay(2)); | ||
assertTrue(updatedTripTimes.getDepartureTime(1) < updatedTripTimes.getArrivalTime(2)); | ||
|
||
// REQUIRED propagation type should never set NO_DATA flags' | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're mixing several features here, aren't you?
Can you please work in small increments? The first PR should only deal with the case where either arrival or departure is missing. In such a case you can use the other one in its place. This shouldn't need any new interpolation logic.
Once we have finished that PR we can look at more complex cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I don't get it then and I think I need to see drawing then.
Using an arrival/departure fallback is, IMO, still a wise first step and something I would like to see first, even if it doesn't fix all of your problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing interpolation logic isn't what you describe. The existing logic is that if the departure time is missing, it is assumed that the bus will always stop for its original dwell time even if it is late (resulting in negative times down the trip), so what I have written and what you are describing are both changing the logic, which is an integral part of this PR.