From 863c0cd8ee769e049b082e1227427efa95244a49 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Fri, 17 Jan 2025 11:23:18 +0000 Subject: [PATCH 1/7] 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/7] 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/7] 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' From 1a09dcede9e3a6b4fa23cde77e13e07aaee00365 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Tue, 28 Jan 2025 10:34:07 +0000 Subject: [PATCH 4/7] apply review suggestion Co-authored-by: Thomas Gran --- .../updater/trip/moduletests/delay/DelayedTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 35bfa22101a..84642152927 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 @@ -75,7 +75,8 @@ void complexDelay() { var tripInput = TripInput .of(TRIP_2_ID) .addStop(STOP_A1, "0:00:00", "0:00:00") - .addStop(STOP_B1, "0:05:00", "0:10:00") // 5-minute dwell + // 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(); From f121b4507fcc4adb40ab152b4caf52b01896d8f8 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Wed, 12 Feb 2025 10:20:51 +0000 Subject: [PATCH 5/7] add new test --- .../trip/moduletests/delay/DelayedTest.java | 58 ++++++++++++++++--- 1 file changed, 50 insertions(+), 8 deletions(-) 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 f9d480ad874..ed1fd412775 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,19 +74,16 @@ void singleStopDelay() { void complexDelay() { var tripInput = TripInput .of(TRIP_2_ID) - .addStop(STOP_A1, "0:00:00", "0:00:00") - // 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") + .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") .build(); var env = RealtimeTestEnvironment.of().addTrip(tripInput).build(); var tripUpdate = new TripUpdateBuilder(TRIP_2_ID, SERVICE_DATE, SCHEDULED, TIME_ZONE) .addDelayedStopTime(0, 0) - .addDelayedArrivalStopTime(1, 900) // 00:20 arr - .addDelayedStopTime(2, 540) // 00:24 arr / 00:25 dep - .addDelayedDepartureStopTime(3, 420) // 00:27 dep + .addDelayedStopTime(1, 60, 80) + .addDelayedStopTime(2, 90, 90) .build(); assertSuccess(env.applyTripUpdate(tripUpdate)); @@ -121,6 +118,50 @@ void complexDelay() { "Original trip should be found in time table for service date" ); + assertEquals( + "SCHEDULED | A1 0:01 0:01:01 | B1 0:01:10 0:01:11 | C1 0:01:20 0:01:21", + 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", + env.getRealtimeTimetable(TRIP_2_ID) + ); + } + + @Test + void delayedAfterNextStopDeparture() { + var tripInput = TripInput + .of(TRIP_2_ID) + .addStop(STOP_A1, "0:00:00", "0:00:00") + // 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.of().addTrip(tripInput).build(); + + var tripUpdate = new TripUpdateBuilder(TRIP_2_ID, SERVICE_DATE, SCHEDULED, TIME_ZONE) + .addDelayedStopTime(0, 0) + .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)); + + var snapshot = env.getTimetableSnapshot(); + + var trip2 = env.getTransitService().getTrip(id(TRIP_2_ID)); + var originalTripPattern = env.getTransitService().findPattern(trip2); + + var originalTimetableScheduled = snapshot.resolve(originalTripPattern, null); + + final int originalTripIndexScheduled = originalTimetableScheduled.getTripIndex(TRIP_2_ID); + var originalTripTimesScheduled = originalTimetableScheduled.getTripTimes( + originalTripIndexScheduled + ); + assertEquals(RealTimeState.SCHEDULED, originalTripTimesScheduled.getRealTimeState()); + assertEquals( "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) @@ -131,3 +172,4 @@ void complexDelay() { ); } } + From 9a7bf17e5cc6594ba94300028d77c0fd862e32e9 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Wed, 12 Feb 2025 10:37:17 +0000 Subject: [PATCH 6/7] add new test --- .../updater/trip/moduletests/delay/DelayedTest.java | 1 - 1 file changed, 1 deletion(-) 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 ed1fd412775..592171566af 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 @@ -172,4 +172,3 @@ void delayedAfterNextStopDeparture() { ); } } - From a1db5e5e5063ec9c40c4e0a6a325ca1c1062351c Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Fri, 21 Feb 2025 10:56:37 +0000 Subject: [PATCH 7/7] formatting --- .../opentripplanner/updater/trip/TripTimesUpdater.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/updater/trip/TripTimesUpdater.java b/application/src/main/java/org/opentripplanner/updater/trip/TripTimesUpdater.java index c7865f4019b..2eb430de8a0 100644 --- a/application/src/main/java/org/opentripplanner/updater/trip/TripTimesUpdater.java +++ b/application/src/main/java/org/opentripplanner/updater/trip/TripTimesUpdater.java @@ -153,8 +153,12 @@ public static Result createUpdatedTripTimesFromGTFS newTimes.setNoData(i); } else { // Else the status is SCHEDULED, update times as needed. - GtfsRealtime.TripUpdate.StopTimeEvent arrival = update.hasArrival() ? update.getArrival() : null; - GtfsRealtime.TripUpdate.StopTimeEvent departure = update.hasDeparture() ? update.getDeparture() : null; + GtfsRealtime.TripUpdate.StopTimeEvent arrival = update.hasArrival() + ? update.getArrival() + : null; + GtfsRealtime.TripUpdate.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