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.

Sign up for GitHub

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

Fix timetable generation for StopTimeUpdate containing only arrival or departure #6392

Open
wants to merge 5 commits into
base: dev-2.x
Choose a base branch
from
+104 −26
Open

Fix timetable generation for StopTimeUpdate containing only arrival or departure #6392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
863c0cd
add test for #6391
miklcct Jan 17, 2025
0733694
fix departure / arrival only StopTimeUpdates causing negative dwell /…
miklcct Jan 17, 2025
8aa9889
update test on the new behaviour
miklcct Jan 17, 2025
1a09dce
apply review suggestion
miklcct Jan 28, 2025
e29e4a1
Merge branch 'dev-2.x' into arrival-only-tripupdate
miklcct Jan 30, 2025
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
52 changes: 44 additions & 8 deletions application/src/main/java/org/opentripplanner/model/Timetable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,9 @@ public Result<TripTimesPatch, UpdateError> createUpdatedTripTimesFromGTFSRT(
StopTimeUpdate update = updates.next();

int numStops = newTimes.getNumStops();
@Nullable
Integer delay = null;
@Nullable
Integer firstUpdatedIndex = null;

final long today = ServiceDateUtils
Expand Down Expand Up @@ -246,11 +248,18 @@ public Result<TripTimesPatch, UpdateError> createUpdatedTripTimesFromGTFSRT(
newTimes.setNoData(i);
} else {
// Else the status is SCHEDULED, update times as needed.
if (update.hasArrival()) {
StopTimeEvent arrival = update.hasArrival() ? update.getArrival() : null;
StopTimeEvent departure = update.hasDeparture() ? update.getDeparture() : null;

// This extra variable is necessary if the departure is specified but the arrival isn't.
// We want to propagate the arrival delay from the previous stop, even if the departure
// delay at this stop is different.
var previousDelay = delay;

if (arrival != null) {
if (firstUpdatedIndex == null) {
firstUpdatedIndex = i;
}
StopTimeEvent arrival = update.getArrival();
if (arrival.hasDelay()) {
delay = arrival.getDelay();
if (arrival.hasTime()) {
Expand All @@ -269,15 +278,12 @@ public Result<TripTimesPatch, UpdateError> createUpdatedTripTimesFromGTFSRT(
);
return Result.failure(new UpdateError(feedScopedTripId, INVALID_ARRIVAL_TIME, i));
}
} else if (delay != null) {
newTimes.updateArrivalDelay(i, delay);
}

if (update.hasDeparture()) {
if (departure != null) {
if (firstUpdatedIndex == null) {
firstUpdatedIndex = i;
}
StopTimeEvent departure = update.getDeparture();
if (departure.hasDelay()) {
delay = departure.getDelay();
if (departure.hasTime()) {
Expand All @@ -296,8 +302,38 @@ public Result<TripTimesPatch, UpdateError> createUpdatedTripTimesFromGTFSRT(
);
return Result.failure(new UpdateError(feedScopedTripId, INVALID_DEPARTURE_TIME, i));
}
} else if (delay != null) {
newTimes.updateDepartureDelay(i, delay);
}

// propagate arrival and departure times, taking care not to cause negative dwells / hops
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

if (arrival == null) {
// propagate the delay from the previous stop
if (previousDelay != null) {
newTimes.updateArrivalDelay(i, previousDelay);
}
// if the arrival time is later than the departure time, set it to the departure time
if (departure != null && newTimes.getArrivalTime(i) > newTimes.getDepartureTime(i)) {
newTimes.updateArrivalTime(i, newTimes.getDepartureTime(i));
}
}

previousDelay = newTimes.getArrivalDelay(i);
if (departure == null) {
if (previousDelay < 0) {
// if the bus is early, only propagate if it is not a timepoint, otherwise assume that
// the bus will wait until the scheduled time
if (newTimes.isTimepoint(i)) {
newTimes.updateDepartureDelay(i, 0);
} else {
newTimes.updateDepartureDelay(i, previousDelay);
}
} else {
// the bus is late, depart as soon as it can after the scheduled time
newTimes.updateDepartureTime(
i,
Math.max(newTimes.getArrivalTime(i), newTimes.getScheduledDepartureTime(i))
);
}
delay = newTimes.getDepartureDelay(i);
}
}

Expand Down
29 changes: 18 additions & 11 deletions application/src/test/java/org/opentripplanner/model/TimetableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Will this test fail if you don't change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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));
});
}

Expand Down Expand Up @@ -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'
Expand Down
32 changes: 32 additions & 0 deletions application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,30 @@ public TripUpdateBuilder addDelayedStopTime(int stopSequence, int delay) {
return addStopTime(null, -1, stopSequence, delay, delay, DEFAULT_SCHEDULE_RELATIONSHIP, null);
}

public TripUpdateBuilder addDelayedArrivalStopTime(int stopSequence, int arrivalDelay) {
return addStopTime(
null,
-1,
stopSequence,
arrivalDelay,
NO_DELAY,
DEFAULT_SCHEDULE_RELATIONSHIP,
null
);
}

public TripUpdateBuilder addDelayedDepartureStopTime(int stopSequence, int departureDelay) {
return addStopTime(
null,
-1,
stopSequence,
NO_DELAY,
departureDelay,
DEFAULT_SCHEDULE_RELATIONSHIP,
null
);
}

public TripUpdateBuilder addDelayedStopTime(
int stopSequence,
int arrivalDelay,
Expand Down Expand Up @@ -166,6 +190,14 @@ private TripUpdateBuilder addStopTime(
departureBuilder.setDelay(departureDelay);
}

if (!arrivalBuilder.hasTime() && !arrivalBuilder.hasDelay()) {
stopTimeUpdateBuilder.clearArrival();
}

if (!departureBuilder.hasTime() && !departureBuilder.hasDelay()) {
stopTimeUpdateBuilder.clearDeparture();
}

return this;
}

Expand Down
17 changes: 10 additions & 7 deletions ...ication/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/DelayedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,19 @@ void singleStopDelay() {
void complexDelay() {
var tripInput = TripInput
.of(TRIP_2_ID)
.addStop(STOP_A1, "0:01:00", "0:01:01")
.addStop(STOP_B1, "0:01:10", "0:01:11")
.addStop(STOP_C1, "0:01:20", "0:01:21")
.addStop(STOP_A1, "0:00:00", "0:00:00")
Copy link
Member

Choose a reason for hiding this comment

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

Please don't modify existing tests and create new ones instead that test this very feature.

// 5-minute dwell
.addStop(STOP_B1, "0:05:00", "0:10:00")
.addStop(STOP_C1, "0:15:00", "0:16:00")
.addStop(STOP_D1, "0:20:00", "0:20:00")
.build();
var env = RealtimeTestEnvironment.gtfs().addTrip(tripInput).build();

var tripUpdate = new TripUpdateBuilder(TRIP_2_ID, SERVICE_DATE, SCHEDULED, TIME_ZONE)
.addDelayedStopTime(0, 0)
.addDelayedStopTime(1, 60, 80)
.addDelayedStopTime(2, 90, 90)
.addDelayedArrivalStopTime(1, 900) // 00:20 arr
.addDelayedStopTime(2, 540) // 00:24 arr / 00:25 dep
.addDelayedDepartureStopTime(3, 420) // 00:27 dep
.build();

assertSuccess(env.applyTripUpdate(tripUpdate));
Expand Down Expand Up @@ -119,11 +122,11 @@ void complexDelay() {
);

assertEquals(
"SCHEDULED | A1 0:01 0:01:01 | B1 0:01:10 0:01:11 | C1 0:01:20 0:01:21",
"SCHEDULED | A1 0:00 0:00 | B1 0:05 0:10 | C1 0:15 0:16 | D1 0:20 0:20",
env.getScheduledTimetable(TRIP_2_ID)
);
assertEquals(
"UPDATED | A1 0:01 0:01:01 | B1 0:02:10 0:02:31 | C1 0:02:50 0:02:51",
"UPDATED | A1 0:00 0:00 | B1 0:20 0:20 | C1 0:24 0:25 | D1 0:27 0:27",
env.getRealtimeTimetable(TRIP_2_ID)
);
}
Expand Down
Loading