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 3 commits into
base: dev-2.x
Choose a base branch
from
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
3 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
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
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);
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
16 changes: 9 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,18 @@ 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")
.addStop(STOP_B1, "0:05:00", "0:10:00") // 5-minute dwell
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.addStop(STOP_B1, "0:05:00", "0:10:00") // 5-minute dwell
// 5-minute dwell
.addStop(STOP_B1, "0:05:00", "0:10:00")

Trailing comments are noe allowed according to over coding guidelines.

.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 +121,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