From 863c0cd8ee769e049b082e1227427efa95244a49 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Fri, 17 Jan 2025 11:23:18 +0000 Subject: [PATCH 1/3] add test for #6391 --- .../updater/trip/TripUpdateBuilder.java | 32 +++++++++++++++++++ .../trip/moduletests/delay/DelayedTest.java | 16 ++++++---- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java b/application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java index e8218edfc1f..5df6c202d9c 100644 --- a/application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java +++ b/application/src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java @@ -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, @@ -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; } diff --git a/application/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/DelayedTest.java b/application/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/DelayedTest.java index 665c79d193c..35bfa22101a 100644 --- a/application/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/DelayedTest.java +++ b/application/src/test/java/org/opentripplanner/updater/trip/moduletests/delay/DelayedTest.java @@ -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 + .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)); @@ -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) ); } From 0733694830d1b4bb4fa834a5f1696b565a9c8a87 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Fri, 17 Jan 2025 14:05:14 +0000 Subject: [PATCH 2/3] fix departure / arrival only StopTimeUpdates causing negative dwell / hop errors --- .../org/opentripplanner/model/Timetable.java | 52 ++++++++++++++++--- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/model/Timetable.java b/application/src/main/java/org/opentripplanner/model/Timetable.java index c437a60b0d5..05d643f6f5c 100644 --- a/application/src/main/java/org/opentripplanner/model/Timetable.java +++ b/application/src/main/java/org/opentripplanner/model/Timetable.java @@ -207,7 +207,9 @@ public Result createUpdatedTripTimesFromGTFSRT( StopTimeUpdate update = updates.next(); int numStops = newTimes.getNumStops(); + @Nullable Integer delay = null; + @Nullable Integer firstUpdatedIndex = null; final long today = ServiceDateUtils @@ -246,11 +248,18 @@ public Result 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()) { @@ -269,15 +278,12 @@ public Result 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()) { @@ -296,8 +302,38 @@ public Result 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); } } From 8aa9889f206faef616c4c6abc83b786f593b7de7 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Fri, 17 Jan 2025 14:51:30 +0000 Subject: [PATCH 3/3] update test on the new behaviour --- .../opentripplanner/model/TimetableTest.java | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/application/src/test/java/org/opentripplanner/model/TimetableTest.java b/application/src/test/java/org/opentripplanner/model/TimetableTest.java index a615041d7a1..78f433adc62 100644 --- a/application/src/test/java/org/opentripplanner/model/TimetableTest.java +++ b/application/src/test/java/org/opentripplanner/model/TimetableTest.java @@ -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); @@ -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'