Interpolate increasing stop times for GTFS-RT cancelled trips #5348

wangyuxuan-symun · 2023-09-11T14:29:08Z

Summary

Relaxed the non-increasing time check when applying real-time updates for cancelled stops. Edit: removed the no_data case.

Issue

Issue 5286
#5286

Unit tests

Added 4 test cases in TripTimesTest class. You can find the explanation and expected results in the corresponding Javadoc.

All other existing tests passed.

Documentation

Modified the Javadoc for validateNonIncreasingTimes method to reflect the change.

…mes. Added test cases for stop cancellations and no_data cases.

codecov · 2023-09-11T14:45:47Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (414cfd4) 66.85% compared to head (f736632) 66.87%.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5348      +/-   ##
=============================================
+ Coverage      66.85%   66.87%   +0.02%     
- Complexity     15651    15661      +10     
=============================================
  Files           1817     1817              
  Lines          70158    70201      +43     
  Branches        7379     7385       +6     
=============================================
+ Hits           46903    46946      +43     
- Misses         20798    20799       +1     
+ Partials        2457     2456       -1

Files	Coverage Δ
...main/java/org/opentripplanner/model/Timetable.java	`81.52% <100.00%> (+0.75%)`	⬆️
...pplanner/updater/trip/TimetableSnapshotSource.java	`69.12% <100.00%> (ø)`
...tripplanner/transit/model/timetable/TripTimes.java	`84.96% <97.56%> (+2.32%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leonardehrenfried · 2023-09-11T16:02:05Z

I think we don't enforce that NO_DATA cannot have arrival/departure time and the spec says that you should not include times for those stop times.

This means that in the OTP model those stop times do have a time, are routable and that can indeed lead to time traveling situations.

src/main/java/org/opentripplanner/transit/model/timetable/TripTimes.java

wangyuxuan-symun · 2023-09-12T11:50:06Z

I think we don't enforce that NO_DATA cannot have arrival/departure time and the spec says that you should not include times for those stop times.

This means that in the OTP model those stop times do have a time, are routable and that can indeed lead to time traveling situations.

I added the NO_DATA case because locally we use NO_DATA when the onboard computer fails to select the correct trip or when the driver forgets to log in. In this case, there is no real-time data applicable. Should I remove the NO_DATA case? Or maybe we can also add a field in the API results to indicate if the stop is cancelled or has no real-time data?

leonardehrenfried · 2023-09-12T12:01:48Z

Yes, please remove the no_data case. No_data to OTP means that there is no realtime data available but the stop still has a scheduled time which we will display to the user and use in routing. For that reason it must be increasing.

Each stop time already has a realtime state field, which contains this exact information.

Which API do you use? https://docs.opentripplanner.org/en/dev-2.x/Apis/

…Times.java Co-authored-by: Thomas Gran <t2gran@gmail.com>

leonardehrenfried · 2023-09-12T12:50:46Z

You need to run mvn prettier:write to format your code.

leonardehrenfried

This looks good to me.

@lassetyr is responsible for the Siri realtime updater and he will check if this is has any effect there.

lassetyr · 2023-09-13T09:04:40Z

It looks like this is based on the assumption that a stop has either

Valid dwell-time at a stop (arrival <= departure)
Cancellation, and is missing realtime-data ("...since delays are propagated to later stops without arrival or departure")

We have seen some corner-cases in SIRI realtime where a cancelled stop may have invalid/negative dwell-time - i.e. the realtime data is present, but it may be wrong/negative.

When this happens, the trip-search breaks with this change.

Described by adjusting one of the tests:

updatedTripTimes.updateDepartureTime(5, 480);

// Negative dwell-time on cancelled stop #6
updatedTripTimes.updateArrivalTime(6, 420);
updatedTripTimes.updateDepartureTime(6, 410);
updatedTripTimes.setCancelled(6);

updatedTripTimes.updateArrivalTime(7, 490);

The negative dwell-time on stop 6 is ignored since it is cancelled (as expected), so the trip now describes a trip back in time between stop 5 and 6.

Finally, when doing a trip-search from 5 to 6, I get the result:
Exception while fetching data (/trip) : This operation is not defined for negative numbers.

leonardehrenfried · 2023-09-13T10:38:15Z

But how can you travel from 5 to 6 when 6 is cancelled? I thought that cancelled stops are excluded from RAPTOR.

wangyuxuan-symun · 2023-09-13T14:03:19Z

Yeah, these assumptions are from GTFS specs and the current GTFS update implementations from OTP, because I am not familiar with the SIRI updates.

So, I manually created a SIRI update for one trip with some local data. However, I couldn't replicate the issue you mentioned. In the example, stops 18 to 32 are cancelled, I set the times between 18 and 31 to 00:00:00 which has negative hop times from stop 17, and 32 has a negative dwell time while being cancelled. If I plan a trip from stop 17 to stop 32, OTP shows me to alight at stop 33 and walk back.

The SIRI update:

<RecordedCall>
  <StopPointRef>52453</StopPointRef>
  <Order>17</Order>
  <Cancellation>false</Cancellation>
  <ActualArrivalTime>06:42:03</ActualArrivalTime>
  <ActualDepartureTime>06:44:44</ActualDepartureTime>
</RecordedCall>
<RecordedCall>
  <StopPointRef>52417</StopPointRef>
  <Order>18</Order>
  <Cancellation>true</Cancellation>
  <ActualArrivalTime>00:00:00</ActualArrivalTime>
  <ActualDepartureTime>00:00:00</ActualDepartureTime>
  <ArrivalStatus>cancelled</ArrivalStatus>
  <ArrivalBoardingActivity>noAlighting</ArrivalBoardingActivity>
  <DepartureStatus>cancelled</DepartureStatus>
  <DepartureBoardingActivity>noBoarding</DepartureBoardingActivity>
</RecordedCall>
...
<RecordedCall>
  <StopPointRef>52005</StopPointRef>
  <Order>31</Order>
  <Cancellation>true</Cancellation>
  <ActualArrivalTime>00:00:00</ActualArrivalTime>
  <ActualDepartureTime>00:00:00</ActualDepartureTime>
  <ArrivalStatus>cancelled</ArrivalStatus>
  <ArrivalBoardingActivity>noAlighting</ArrivalBoardingActivity>
  <DepartureStatus>cancelled</DepartureStatus>
  <DepartureBoardingActivity>noBoarding</DepartureBoardingActivity>
</RecordedCall>
<RecordedCall>
  <StopPointRef>51972</StopPointRef>
  <Order>32</Order>
  <Cancellation>true</Cancellation>
  <ActualArrivalTime>07:01:00</ActualArrivalTime>
  <ActualDepartureTime>07:00:00</ActualDepartureTime>
  <ArrivalStatus>cancelled</ArrivalStatus>
  <ArrivalBoardingActivity>noAlighting</ArrivalBoardingActivity>
  <DepartureStatus>cancelled</DepartureStatus>
  <DepartureBoardingActivity>noBoarding</DepartureBoardingActivity>
</RecordedCall>
<RecordedCall>
  <StopPointRef>51937</StopPointRef>
  <Order>33</Order>
  <Cancellation>false</Cancellation>
  <ActualArrivalTime>07:01:13</ActualArrivalTime>
  <ActualDepartureTime>07:01:13</ActualDepartureTime>
</RecordedCall>

The legs of my itinerary in JSON

            {
              "from": {
                "name": "Origin",
                "stopPosition": null
              },
              "to": {
                "name": "du Mont-Royal / Fullum",
                "stopPosition": null
              },
              "mode": "WALK",
              "duration": 1
            },
            {
              "from": {
                "name": "du Mont-Royal / Fullum",
                "stopPosition": {
                  "position": 17
                }
              },
              "to": {
                "name": "du Mont-Royal / Saint-Urbain",
                "stopPosition": {
                  "position": 33
                }
              },
              "mode": "BUS",
              "duration": 989
            },
            {
              "from": {
                "name": "du Mont-Royal / Saint-Urbain",
                "stopPosition": null
              },
              "to": {
                "name": "Destination",
                "stopPosition": null
              },
              "mode": "WALK",
              "duration": 143
            }

leonardehrenfried · 2023-09-14T09:09:19Z

If I plan a trip from stop 17 to stop 32, OTP shows me to alight at stop 33 and walk back.

This is the result we want, isn't it? All the stops in between are cancelled so walking back is the only option.

lassetyr · 2023-09-14T11:25:11Z

But how can you travel from 5 to 6 when 6 is cancelled? I thought that cancelled stops are excluded from RAPTOR.

I did a trip-search from 5 to 6 - Note: I also had set includeRealtimeCancellations:true in my query. As a client, I can't know that 6 is cancelled. The error seems to occur in the heuristic-search.

Trip-query: https://gist.github.com/lassetyr/02eca07c0e70d650a5bda7fb3cd3b0e9
Response: https://gist.github.com/lassetyr/572c9d33bde0dd30b0658bd9caaa98e6
(TransmodelGraphQL-API)

Stops/calls in a complete SIRI-update - as usage has been defined in EU - always has both arrival- and departureTimes - the complete SIRI-update that resulted in the error can be found at: https://gist.github.com/lassetyr/d9890372b03291706491de62130983dd

Stop 1: 20 minutes delay
Stop 2: Cancelled, negative hop-time (1-2) and negative dwell-time (arrival - departure)
Stop 3: 20 minutes delay

(Sending these negative values is not allowed, but we still see that it happens)

Full NeTEx-dataset loaded: https://storage.googleapis.com/marduk-production/outbound/netex/rb_norway-aggregated-netex.zip

I now also tested sending different versions of the SIRI-update with valid/invalid times. This causes the error to eventually go away - even if invalid data is sent. With a full restart of OTP and reapplying the invalid data, it reappears. I have not looked into why this happens...

Is there a unit/integration test that tests trip-searches with applied realtime-data?

leonardehrenfried · 2023-09-14T14:21:43Z

@wangyuxuan-symun As you're aware we debated this and while we haven't come to a firm conclusion we have a couple of ideas. One of them would be to propagate the delay to the cancelled stops, however there were also concerns about simply inventing a time.

We will be thinking about this a little more and hope to come back to this problem.

wangyuxuan-symun · 2023-09-14T17:58:03Z

Sorry that I had to leave early...

Personally, I prefer to either apply the entire update or ignore the entire update. Otherwise, the time variable would have two meanings: scheduled and real-time at the same time, which is confusing and conflicting. For canceled stops, we drop the times from our electronic display boards. The apps would show the scheduled time but greyed out with a line crossing it out so passengers know which trip is affected.

I don't like the idea of inventing a time. If we really want to invent a time, I think delays have to be interpolated between the non-cancelled stop before and the non-cancelled stop after. We have a lot of stop cancellations where the buses take the highway to skip the regular route, which will catch up with the delays and end up running early. Thus, if we apply the same delay to the canceled stops, we will still end up with non-increasing times...

FYI, I will be on vacation for the next few weeks, so my response time will be slower than usual.

leonardehrenfried · 2023-09-22T08:19:55Z

We had a quick discussion about this at the last dev meeting.

Since we have the requirement of Siri trips that have to be rejected when the time on their cancelled stops are not increasing we are left with only one choice: propagate the delay forward to the cancelled stops so that they, too, become increasing.

@optionsome is the expert for the delay propagation. Could you point out where this happens?

optionsome · 2023-09-22T08:26:10Z

For forwards direction (this is what the specification says should happen) https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/main/java/org/opentripplanner/model/Timetable.java#L311-L312 and backwards (this is something we are forced to do to ensure the timetable is usable in routing) https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/main/java/org/opentripplanner/model/Timetable.java#L323-L344

leonardehrenfried · 2023-09-22T08:30:29Z

Just so I get this right: it is also supposed to work in the absence of the delay field, isn't it?

leonardehrenfried · 2023-10-10T11:27:23Z

src/main/java/org/opentripplanner/transit/model/timetable/TripTimes.java

+   * Interpolate the times for CANCELLED stops in between regular stops to ensure internal time
+   * representations are increasing for these stops. This will not interpolate terminal
+   * cancellations, as those can be handled by backward and forward propagations. Note: This is
+   * currently for GTFS-RT only, since SIRI requires times for all stops.


Suggested change

* Interpolate the times for CANCELLED stops in between regular stops to ensure internal time

* representations are increasing for these stops. This will not interpolate terminal

* cancellations, as those can be handled by backward and forward propagations. Note: This is

* currently for GTFS-RT only, since SIRI requires times for all stops.

* Interpolate the times for CANCELLED stops in between regular stops to ensure internal time

* representations are increasing for these stops. This will not interpolate terminal

* cancellations, as those can be handled by backward and forward propagations. Note: This is

* currently for GTFS-RT only, since SIRI requires times for all stops and there we _want_ to reject trips

* where the times for cancelled stops are missing or are non-increasing.

leonardehrenfried · 2023-10-10T11:41:13Z

src/test/java/org/opentripplanner/model/TimetableTest.java

+    var tripDescriptorBuilder = tripDescriptorBuilder(TRIP_ID);
+    TripUpdate.Builder tripUpdateBuilder = TripUpdate.newBuilder();
+    tripUpdateBuilder.setTrip(tripDescriptorBuilder);
+
+    // Stop 1
+    StopTimeUpdate.Builder stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(0);
+    stopTimeUpdateBuilder.setStopSequence(1);
+    stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SCHEDULED);
+    StopTimeEvent.Builder stopTimeEventBuilder = stopTimeUpdateBuilder.getArrivalBuilder();
+    stopTimeEventBuilder.setDelay(0);
+    stopTimeEventBuilder = stopTimeUpdateBuilder.getDepartureBuilder();
+    stopTimeEventBuilder.setDelay(0);
+
+    // Stop 2
+    stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(1);
+    stopTimeUpdateBuilder.setStopSequence(2);
+    stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SKIPPED);
+
+    // Stop 3
+    stopTimeUpdateBuilder = tripUpdateBuilder.addStopTimeUpdateBuilder(2);
+    stopTimeUpdateBuilder.setStopSequence(3);
+    stopTimeUpdateBuilder.setScheduleRelationship(StopTimeUpdate.ScheduleRelationship.SCHEDULED);
+    stopTimeEventBuilder = stopTimeUpdateBuilder.getArrivalBuilder();
+    stopTimeEventBuilder.setDelay(-800);
+    stopTimeEventBuilder = stopTimeUpdateBuilder.getDepartureBuilder();
+    stopTimeEventBuilder.setDelay(-800);
+
+    TripUpdate tripUpdate = tripUpdateBuilder.build();


I think you can reduce the duplication in this code a lot by using the TripUpdateBuilder like this:

OpenTripPlanner/src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java

Lines 507 to 519 in b5d1a9c

var builder = new TripUpdateBuilder(

scheduledTripId,

SERVICE_DATE,

SCHEDULED,

transitModel.getTimeZone()

)

.addDelayedStopTime(1, 0)

.addDelayedStopTime(2, 60, 80)

.addDelayedStopTime(3, 90, 90);

var tripUpdate = builder.build();

var updater = defaultUpdater();

If there are methods missing in that class you can add them.

leonardehrenfried

The logic from of the GTFS side is good but I have left a few comments.

t2gran · 2023-10-13T11:10:45Z

Consider configuring this feature per updater. This should be default off. If this is done, then no explicit exclude for SIRI is needed. Only GTFS updeters need the flag to turn it on.

…nding test cases

No conflict to resolve.

wangyuxuan-symun · 2023-10-26T18:48:55Z

Hello! Sorry for the late update, I was sick for the past two weeks...

Based on the earlier discussion, I moved the interpolation method to the Timetable class, which is the GTFS-RT updater. Test cases are moved as well. I also set the method to private, so no explicit exclusion is needed in the documentation. However, I didn't add a new flag to turn it on or off, because the current GTFS updater ignores SKIPPED stop times even if they have times. If we decide to create a generic updater layer later as mentioned in the TODO section, I think maybe we can add the flag at that time.

Also I have a question for @leonardehrenfried about the TripUpdateBuilder class. For the method addStopTime without any delays values on line 84. The delay values are set to NO_VALUE, which is -1. However, other methods will default to NO_DELAY, which is Integer.MIN_VALUE. If I add a skipped stop using NO_VALUE, the result TripUpdate will show delay as -1. If I use NO_DELAY, then the arrival and departure will be empty. Is this intentional? I created a small addSkippedStop for now, I am happy to remove it later if you can clarify this question. Thank you!

leonardehrenfried · 2023-10-30T09:52:00Z

src/test/java/org/opentripplanner/model/TimetableTest.java

+    assertTrue(patch.isSuccess());
+
+    patch.ifSuccess(p -> {
+      var updatedTripTimes = p.getTripTimes();


Can you please run the validateNonIncreasingTimes in the test to make absolutely sure that they are increasing?

The validateNonIncreasingTimes is called at the end of createUpdatedTripTimes method, and the interpolation is done before validating times. If the times are not increasing, the assertTrue you referenced here will fail.

// Interpolate missing times from CANCELLED stops. Note: Currently for GTFS-RT ONLY. if (interpolateMissingTimes(newTimes)) { LOG.debug("Interpolated delays for cancelled stops on trip {}.", tripId); } // Validate for non-increasing times. Log error if present. var error = newTimes.validateNonIncreasingTimes();

Ah, I see. that's ok then.

leonardehrenfried · 2023-10-30T09:57:08Z

Also I have a question for @leonardehrenfried about the TripUpdateBuilder class. For the method addStopTime without any delays values on line 84. The delay values are set to NO_VALUE, which is -1. However, other methods will default to NO_DELAY, which is Integer.MIN_VALUE. If I add a skipped stop using NO_VALUE, the result TripUpdate will show delay as -1. If I use NO_DELAY, then the arrival and departure will be empty. Is this intentional? I created a small addSkippedStop for now, I am happy to remove it later if you can clarify this question. Thank you!

That's an oversight. If I replace NO_VALUE with NO_DELAY all the tests still pass. You can modify this method and delete yours.

leonardehrenfried · 2023-10-30T10:01:41Z

Based on the earlier discussion, I moved the interpolation method to the Timetable class, which is the GTFS-RT updater. Test cases are moved as well. I also set the method to private, so no explicit exclusion is needed in the documentation. However, I didn't add a new flag to turn it on or off, because the current GTFS updater ignores SKIPPED stop times even if they have times. If we decide to create a generic updater layer later as mentioned in the TODO section, I think maybe we can add the flag at that time.

When we talked about this in the meeting, @optionsome and @t2gran felt that it was against the GTFS-RT spec to propagate delays on skipped stop times, that's why they requested this flag. I personally can't find anything in the spec that states this. @optionsome, you're usually quite good about this - can you show us the language in the spec that talks about this topic?

optionsome · 2023-10-30T11:18:36Z

When we talked about this in the meeting, @optionsome and @t2gran felt that it was against the GTFS-RT spec to propagate delays on skipped stop times, that's why they requested this flag. I personally can't find anything in the spec that states this. @optionsome, you're usually quite good about this - can you show us the language in the spec that talks about this topic?

I re-read the specification and it doesn't seem to explicitly touch this subject. The closest thing I could find was "Note that updates with a schedule relationship of SKIPPED will not stop delay propagation" in https://gtfs.org/realtime/feed-entities/trip-updates/ . This is sort of a weird topic for most contexts since we the vehicle doesn't actually visit the skipped stop so having any estimate for it doesn't really make sense. However, propagating the delay to skipped stops can lead to the departure time of a skipped stop being after the arrival time to the next stop if there is an estimate for the next stop. This can lead to issues if we do routing the include cancellation flag on. Although, the similar issues can exist if we don't propagate the delay.

I'm really not sure if we are doing something according or against the specification if we propagate.

leonardehrenfried · 2023-10-30T11:30:01Z

src/test/java/org/opentripplanner/updater/trip/TripUpdateBuilder.java

+  /**
+   * Add a skipped stop to the TripUpdate.
+   */
+  public TripUpdateBuilder addSkippedStop(int stopSequence) {


Having this method is also fine so no need to remove it if you don't want to.

How about I add another method for NO_DATA cases which also don't have times associated, and remove the addStopTime method I mentioned earlier, so that the StopTimes can be built more explicitly?

Yes, that's a good idea.

wangyuxuan-symun · 2023-11-01T15:25:17Z

Here's the paragraph from the documentation in https://gtfs.org/realtime/feed-entities/trip-updates/.

If one or more stops are missing along the trip the delay from the update (or, if only time is provided in the update, a delay computed by comparing the time against the GTFS schedule time) is propagated to all subsequent stops. This means that updating a stop time for a certain stop will change all subsequent stops in the absence of any other information. Note that updates with a schedule relationship of SKIPPED will not stop delay propagation, but updates with schedule relationships of SCHEDULED (also the default value if schedule relationship is not provided) or NO_DATA will.

I think there are cases where propagation is required, and sometimes propagation is not required. Here's my understanding. Say we have 3 stops, scheduled at 0, 60, 120.

Case 1: If all stops are regular stops, and the trip updates only contain information for stop 1, but not for stop 2 and 3. Hence, in "the absence of any other information", and we don't have "any other information", stop 2 and 3 will get the same delay from stop 1. I think propagation here is required, and this is what OTP does. For example, if the delay for stop 1 is 200 (arrive at 200), stop 2 and 3 will get delayed by 100 (arrive at 260, 320).

Case 2: If stop 1 and 3 are regular stops, stop 2 is cancelled, and the trip updates only contain information for stop 1 and 2, but not for stop 3.From the documentation "updates with a schedule relationship of SKIPPED will not stop delay propagation". I think propagation here is required, we can propagate delays after stop 2, and this is what OTP does. For example, if the delay for stop 1 is 200 (arrive at 200), stop 2 is cancelled (OTP internal representation 260), then 3 will get a propagated delay of 200 (arrive at 320) in the "absent of any other information" for stop 3.

Case 3: If stop 1 and 3 are regular stops, stop 2 has no data, and the trip updates only contain information for stop 1 and 2, but not for stop 3. From the documentation "updates with schedule relationships of SCHEDULED or NO_DATA will" stop propagation. So, propagation is not allowed, we cannot propagate delays after stop 2 absent of any information. This is what OTP does. Example, if the delay for stop 1 is 200 (arrive at 200), stop 2 has no data (OTP defaults to schedule 60), then 3 will get no propagated delay (defaults to schedule 120). So in this case, we will have non-increasing times in the "absent of any other information" for stop 3.

Case 4 (my original issue): If stop 1 and 3 are regular stops, stop 2 is cancelled, and the trip updates contain information for stop 1, 2, and 3. For example, if the delay for stop 1 is 200 (arrive at 200), stop 2 is cancelled (if we propagate delays, arrive at 260), and stop 3 has a delay of 100 (arrive at 220). I agree with @optionsome that in this case

propagating the delay to skipped stops can lead to the departure time of a skipped stop being after the arrival time to the next stop if there is an estimate for the next stop. This can lead to issues if we do routing the include cancellation flag on. Although, the similar issues can exist if we don't propagate the delay.

However, the difference in this case is that we have the "other information" that is absent in previous cases, that's why I think it is fine to not propagate the delays and do something else.
The interpolation method only deals with cancelled stops sandwiched in between normal stops. It is to ensure that the cancelled stop is assigned an internal time between the normal stops before and after the cancellation so that they don't cause any issues in this case. So, this case with interpolation, the times will be stop 1 at 200, stop 2 is cancelled (interpolated to be 210), and stop 3 at 220.
Interpolation is still done if there are issues with the normal times. However, an error will show up when validating non-increasing times (See test case testUpdateWithCancellationAndNonIncreasingTimes)
In case of terminal cancellations at the beginning of the route, the propagation is handled by backward propagation not the interpolation method (test case testUpdateWithStartTerminalCancellationAndEarly). In case of terminal cancellations at the end of the route, the propagation is handled by forward propagation by default not the interpolation method (test case testUpdateWithEndTerminalCancellationAndLate). I can write additional test cases to test forward delay propagation, e.g. case 1 to 3 with a few variations.
*I am trying to make the times look nice, I think it would still be fine if we set the times of cancelled stops to the same time as the regular stop right before. So here, it can be stop 1 at 200, stop 2 is cancelled (set to 200), and stop 3 at 220. If there are more stops in between 1 and 3 and all of them are cancelled, set all of them to 200.

t2gran · 2023-11-01T15:17:54Z

src/main/java/org/opentripplanner/model/Timetable.java

+   *
+   * @return true if there is interpolated times, false if there is no interpolation.
+   */
+  private boolean interpolateMissingTimes(TripTimes newTimes) {


This does not belong here - I think it is best left in the TripTimes with documentation:

NOTE! THIS IS A HACK TO SUPPORT ... GTFS-RT AND OTPs CONSTRAINT THAT ALL STOPS NEED TO HAVE INCREASING TIMES.

This method was in TripTimes in the beginning, but you commented:

Consider configuring this feature per updater. This should be default off. If this is done, then no explicit exclude for SIRI is needed. Only GTFS updeters need the flag to turn it on.

That's why I moved it right next to the GTFS-RT method and made it private. Would you still want me to move it back to TripTimes?

I think there was a misunderstanding: in our language an updater is an individual feed source updater rather than then Siri/Gtfsrt split you thought about.

So, yes, please move it back into TripTimes. Sorry for the confusion.

Ah, I see. I will move everything back then!

t2gran · 2023-11-01T15:27:14Z

src/main/java/org/opentripplanner/model/Timetable.java

@@ -343,6 +352,12 @@ public Result<TripTimesPatch, UpdateError> createUpdatedTripTimes(
      }
    }

+    // Interpolate missing times from CANCELLED stops. Note: Currently for GTFS-RT ONLY.


Suggested change

// Interpolate missing times from CANCELLED stops. Note: Currently for GTFS-RT ONLY.

// Interpolate missing times from CANCELLED stops.

// THIS IS A HACK TO SUPPORT THE OTP RESTRICTON THAT ALL TIMES NEED TO BE INCREASING.

// THIS IS DUE TO THE SUPPORT OF THE `includeRealtimeCancellations`.

// THIS ONLY APPLY TO GTFS, NOT SIRI.

Also the JavaDoc on the method need to be updated. Consider renaming the method to createUpdatedTripTimesForGTFS.

Hopefully we can address this and clean up this mess after the refactoring of the transit model.

…odified TripUpdateBuilder to be more explicit, renamed method to createUpdatedTripTimesFromGTFSRT to avoid confusion with SIRI.

wangyuxuan-symun · 2023-11-03T13:39:09Z

Hello! Here's the latest commit. I moved the interpolation method back to TripTimes, added test cases, modified TripUpdateBuilder to be more explicit, renamed method to createUpdatedTripTimesFromGTFSRT to avoid confusion with SIRI. Let me know if there is anything else I forgot!

wangyuxuan-symun · 2023-11-10T04:01:37Z

Hello again! I just got a notification saying the build has failed. I checked the error log, and it says GraphQLIntegrationTest.setup:123 » ArrayIndexOutOfBounds Index 211 out of bounds for length 31. This line has something to do with Vehicle Parking which I didn't touch and it only failed in Linux, which seems odd... I merged the latest updates to see if I could reproduce the issue, but I am on Windows and everything passed. I am wondering if you have any idea. If not, I can do some more debugging next week.

leonardehrenfried · 2023-11-10T07:01:14Z

We had a problem last week with a annoyingly flaky test. It had nothing to do with your change.

It has since been fixed and your build is green again. @t2gran will review your code when he gets to it.

leonardehrenfried · 2023-11-13T16:55:00Z

Thanks for the contribution, @wangyuxuan-symun!

Added cancellation and no_data checks in validating non-increasing ti…

6990202

…mes. Added test cases for stop cancellations and no_data cases.

wangyuxuan-symun requested a review from a team as a code owner September 11, 2023 14:29

leonardehrenfried added the RealTimeUpdate The issue/PR is related to RealTime updates label Sep 11, 2023

leonardehrenfried added this to the 2.4 (next release) milestone Sep 11, 2023

leonardehrenfried requested review from lassetyr and leonardehrenfried September 12, 2023 09:06

t2gran reviewed Sep 12, 2023

View reviewed changes

src/main/java/org/opentripplanner/transit/model/timetable/TripTimes.java Outdated Show resolved Hide resolved

wangyuxuan-symun and others added 2 commits September 12, 2023 08:37

Remove the NO_DATA cases

67be79a

Update src/main/java/org/opentripplanner/transit/model/timetable/Trip…

1cfe376

…Times.java Co-authored-by: Thomas Gran <t2gran@gmail.com>

wangyuxuan-symun changed the title ~~Relax non-increasing time check for cancelled or no_data stops.~~ Relax non-increasing time check for cancelled stops. Sep 12, 2023

Fixed the formatting - removed a white space at the end of the line...

2afaf0c

leonardehrenfried previously approved these changes Sep 12, 2023

View reviewed changes

leonardehrenfried changed the title ~~Relax non-increasing time check for cancelled stops.~~ Relax non-increasing time check for cancelled stops Sep 12, 2023

t2gran modified the milestones: 2.4, 2.5 (next release) Sep 13, 2023

leonardehrenfried changed the title ~~Relax non-increasing time check for cancelled stops~~ Interpolate increasing stop times for GTFS-RT cancelled trips Oct 10, 2023

leonardehrenfried reviewed Oct 10, 2023

View reviewed changes

leonardehrenfried requested changes Oct 10, 2023

View reviewed changes

wangyuxuan-symun added 2 commits October 26, 2023 14:23

Moved the interpolation method to GTFS Updater, modified the correspo…

140c8cb

…nding test cases

Merge from opentripplanner/dev-2.x 20231026

ecf629c

No conflict to resolve.

leonardehrenfried reviewed Oct 30, 2023

View reviewed changes

leonardehrenfried requested a review from t2gran October 31, 2023 09:51

t2gran requested changes Nov 1, 2023

View reviewed changes

wangyuxuan-symun added 2 commits November 3, 2023 09:27

Merge 20231103 No Conflict

954bb64

Moved the interpolation method back to TripTimes, added test cases, m…

6e85c2f

…odified TripUpdateBuilder to be more explicit, renamed method to createUpdatedTripTimesFromGTFSRT to avoid confusion with SIRI.

leonardehrenfried approved these changes Nov 6, 2023

View reviewed changes

leonardehrenfried requested a review from t2gran November 7, 2023 09:18

wangyuxuan-symun requested a review from leonardehrenfried November 9, 2023 22:15

Merge 20231109

f736632

t2gran approved these changes Nov 13, 2023

View reviewed changes

leonardehrenfried merged commit 0c84169 into opentripplanner:dev-2.x Nov 13, 2023
5 checks passed

t2gran pushed a commit that referenced this pull request Nov 13, 2023

Add changelog entry for #5348 [ci skip]

90bbb73

leonardehrenfried mentioned this pull request Jan 2, 2024

NEGATIVE_DWELL_TIME failure when vehicle leaves the first stop early and no Arrival provided #5347

Closed

	var builder = new TripUpdateBuilder(
	scheduledTripId,
	SERVICE_DATE,
	SCHEDULED,
	transitModel.getTimeZone()
	)
	.addDelayedStopTime(1, 0)
	.addDelayedStopTime(2, 60, 80)
	.addDelayedStopTime(3, 90, 90);

	var tripUpdate = builder.build();

	var updater = defaultUpdater();

-    // Interpolate missing times from CANCELLED stops. Note: Currently for GTFS-RT ONLY.
+    // Interpolate missing times from CANCELLED stops.
+    // THIS IS A HACK TO SUPPORT THE OTP RESTRICTON THAT ALL TIMES NEED TO BE INCREASING.
+    // THIS IS DUE TO THE SUPPORT OF THE `includeRealtimeCancellations`.
+    // THIS ONLY APPLY TO GTFS, NOT SIRI.

Interpolate increasing stop times for GTFS-RT cancelled trips #5348

Interpolate increasing stop times for GTFS-RT cancelled trips #5348

Conversation

wangyuxuan-symun commented Sep 11, 2023 • edited

Summary

Issue

Unit tests

Documentation

codecov bot commented Sep 11, 2023 • edited

Codecov Report

leonardehrenfried commented Sep 11, 2023

wangyuxuan-symun commented Sep 12, 2023

leonardehrenfried commented Sep 12, 2023

leonardehrenfried commented Sep 12, 2023

leonardehrenfried left a comment

Choose a reason for hiding this comment

lassetyr commented Sep 13, 2023

leonardehrenfried commented Sep 13, 2023

wangyuxuan-symun commented Sep 13, 2023

leonardehrenfried commented Sep 14, 2023

lassetyr commented Sep 14, 2023 • edited by leonardehrenfried

leonardehrenfried commented Sep 14, 2023

wangyuxuan-symun commented Sep 14, 2023

leonardehrenfried commented Sep 22, 2023

optionsome commented Sep 22, 2023

leonardehrenfried commented Sep 22, 2023

leonardehrenfried Oct 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

t2gran commented Oct 13, 2023

wangyuxuan-symun commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Oct 30, 2023

leonardehrenfried commented Oct 30, 2023

optionsome commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyuxuan-symun commented Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyuxuan-symun commented Nov 3, 2023

wangyuxuan-symun commented Nov 10, 2023 • edited

leonardehrenfried commented Nov 10, 2023

leonardehrenfried commented Nov 13, 2023

wangyuxuan-symun commented Sep 11, 2023 •

edited

codecov bot commented Sep 11, 2023 •

edited

lassetyr commented Sep 14, 2023 •

edited by leonardehrenfried

leonardehrenfried Oct 10, 2023 •

edited

wangyuxuan-symun commented Nov 1, 2023 •

edited

wangyuxuan-symun commented Nov 10, 2023 •

edited