Add the ability to specify delay or skipped stops on ADDED or REPLACEMENT trips in GTFS-RT #6028

miklcct · 2024-08-28T12:16:06Z

Summary

This PR adds the ability to support delays or skipped stops on ADDED or REPLACEMENT trips. The time is the actual estimated time of the trip, and the scheduled time is the time given minus the delay given.

Issue

Closes #6009 . The proposal is submitted to google/transit#490 as well.

Unit tests

Unit tests are added for TripUpdates which specify both time and delay in a StopTimeEvent. This case was previously assumed to be invalid and hence a new method is added to generate items. Tests are also added for ADDED trips containing skipped stops as well.

Documentation

The original // TODO: should we incorporate the delay field if present? is done by this PR.

codecov · 2024-08-29T10:27:42Z

Codecov Report

Attention: Patch coverage is 78.48101% with 17 lines in your changes missing coverage. Please review.

Project coverage is 69.74%. Comparing base (99998b8) to head (7479b54).
Report is 21 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...rg/opentripplanner/updater/trip/AddedStopTime.java	76.08%	5 Missing and 6 partials ⚠️
...pplanner/updater/trip/TimetableSnapshotSource.java	81.81%	1 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #6028   +/-   ##
==========================================
  Coverage      69.73%   69.74%           
- Complexity     17315    17329   +14     
==========================================
  Files           1960     1960           
  Lines          74267    74308   +41     
  Branches        7603     7606    +3     
==========================================
+ Hits           51789    51823   +34     
- Misses         19834    19835    +1     
- Partials        2644     2650    +6

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

optionsome

ADDED and REPLACEMENT updates don't have many users in the OTP community (I think REPLACEMENT is slowly being replaced by new additions to the GTFS specifications, but I'm not fully up-to-date on the status of those drafts), but we are open to these implementations being fixed and/or improved.

I doubt this will break any existing OTP deployments as I think your interpretation of how delay should be used is the only reasonable one. Similarly, I think previously someone might have just left out a stop in an ADDED trip if it was no longer used but having this possibility to mark them as SKIPPED shouldn't negatively affect anyone. If someone is opposed to these changes, let us know.

The only small question mark is that should we wait for some comments from the GTFS community on your suggestion before this is merged or not. Hopefully, the suggestion to the GTFS specification goes through so there is no longer any gray area around these usages.

optionsome · 2024-08-30T18:53:57Z

src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java

@@ -838,12 +845,30 @@ private Result<UpdateSuccess, UpdateError> addTripToGraphAndBuffer(
 );

 // Update all times to mark trip times as realtime
- // TODO: should we incorporate the delay field if present?
+ // TODO: This is based on the proposal at https://github.com/google/transit/issues/490


I don't know if we should keep the TODO in this comment.

Shall I remove this comment for now, or wait until I further work on the proposal?

optionsome · 2024-08-30T18:57:20Z

src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java

+ final int arrivalDelay = stopTimeUpdate.hasArrival()
+ ? stopTimeUpdate.getArrival().hasDelay() ? stopTimeUpdate.getArrival().getDelay() : 0
+ : 0;
+ final int departureDelay = stopTimeUpdate.hasDeparture()
+ ? stopTimeUpdate.getDeparture().hasDelay() ? stopTimeUpdate.getDeparture().getDelay() : 0
+ : 0;


These are a bit difficult to read. Perhaps using if check for the stopTimeUpdate.hasArrival/Departure would be more readable.

If I use an if statement, the variable can't be made final, and its generally preferred to avoid mutable variable if I can.

I agree with @optionsome. These checks are very hard to read. You should move them into the class AddedStopTime, encapsulate them and write Javadoc.

Can you please do that for all the other checks in this methods even the ones that were already there?

miklcct · 2024-08-30T22:17:28Z

REPLACEMENT is officially deprecated, and the behaviour of ADDED remains unspecified. The proposal addresses DUPLICATED (which has a clearly defined behaviour) as well as ADDED.

Google uses ADDED to duplicate an existing trip to run it at another time, which is now officially migrated to DUPLICATED in the standard, while OpenTripPlanner requires specifying the whole trip and allows arbitrary pattern for ADDED trips, which fits the use case to add trips which are not based on existing patterns (which Google doesn't support).

To my understanding there isn't a way in the official GTFS-RT standard to add arbitrary new realtime trips into a route not based on an existing pattern (even Google doesn't support that). Even in the OpenTripPlanner implementation it is impossible to specify headsigns of added trips (both at trip and stop level) but this will be another issue to solve.

GTFS-TripModification proposal can be used for diverted trips but it isn't supported in OpenTripPlanner yet and it is much more complicated to just replacing the whole trip in case of diversion using the deprecated REPLACEMENT feature of OpenTripPlanner.

I am thinking that we should wait for a while to see how the GTFS community progresses on trip additions / diversions / replacement, but I will have a production deployment in the next few months using this change in order to support real time added / diverted trains, which few other commercial providers support (neither Google nor Citymapper could route me via the official diversion to Battersea Park when the line to Clapham Junction was blocked unexpectedly).

Could you please let me know if there are any existing OpenTripPlanner deployments which do handle real-time diverted trips?

miklcct · 2024-08-30T23:32:53Z

Possibly need further testing for REPLACEMENT trips:

leonardehrenfried · 2024-09-03T12:08:54Z

We talked about this in today's dev meeting (to which you are welcome). We are generally ok with this interpreation of the spec but would like to see it codified in the spec.

Are you interested in un-deprecating the ADDED schedule relationship in the GTFS spec repo? google/transit#490 is a good start but can you create an actual PR which can be discussed and critiqued line by line by the community? To me the most important change I would like to see is an actually description of what ADDED means. The OTP implementation thinks that it allows you to create completely new trips but I would like to clarify.
If you're okay with trying the spec change I would support you with it.

Since the realtime code is old and has incurred lots of technical debt, we would also like to see some clean up. Are you interested in that?

src/test/java/org/opentripplanner/updater/trip/moduletests/addition/AddedTest.java

miklcct · 2024-09-03T12:28:51Z

I will be interested in cleaning up the code if it means a chance for me to get headsigns, shapes, completely new stops, etc., into the realtime updates but I would like to see how the GTFS-ServiceChanges v3.1 goes first because we commit to anything.

A major limitation of the product we are developing is the inability to specify the headsign for modified trips under the current specification, so I am relying on frontend tweaks to "correct" the headsign for trains terminating short.

leonardehrenfried · 2024-09-03T12:30:11Z

I also found a thread on Slack about ADDED: https://mobilitydata-io.slack.com/archives/C3D321CKB/p1715957220876199

I think there will be interest in specifying it.

leonardehrenfried · 2024-09-03T14:32:07Z

Before I review any further: are you going to convert google/transit#490 into a spec PR?

miklcct · 2024-09-03T14:42:12Z

I plan to do so but not in these few days.

src/main/java/org/opentripplanner/updater/trip/AddedStopTime.java

src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java

src/main/java/org/opentripplanner/updater/trip/AddedStopTime.java

miklcct · 2024-09-06T15:19:50Z

I also found a thread on Slack about ADDED: https://mobilitydata-io.slack.com/archives/C3D321CKB/p1715957220876199

I think there will be interest in specifying it.

I have submitted the Google Form but haven't received any invitation link yet.

leonardehrenfried · 2024-09-11T05:32:57Z

@miklcct did you get access to the MB slack?

miklcct · 2024-09-11T09:09:34Z

@miklcct did you get access to the MB slack?

Still no invitation received in my mailbox (checked spam as well as inbox) despite submitted the form multiple times.

leonardehrenfried · 2024-09-11T09:15:17Z

I posted the following message. I hope an admin will help you soon.

regardless of the original PickDrop specified in the MFDZ extension

…stops in added trips

release 2.6.0

miklcct requested a review from a team as a code owner August 28, 2024 12:16

miklcct force-pushed the delay_on_added_trips branch 5 times, most recently from b78e020 to 71efc46 Compare August 28, 2024 12:55

miklcct changed the title ~~Add the ability to specify delay on ADDED or REPLACEMENT trips in GTFS-RT.~~ Add the ability to specify delay or skipped stops on ADDED or REPLACEMENT trips in GTFS-RT. Aug 28, 2024

optionsome self-requested a review August 29, 2024 10:29

optionsome requested changes Aug 30, 2024

View reviewed changes

leonardehrenfried changed the title ~~Add the ability to specify delay or skipped stops on ADDED or REPLACEMENT trips in GTFS-RT.~~ Add the ability to specify delay or skipped stops on ADDED or REPLACEMENT trips in GTFS-RT Aug 31, 2024

leonardehrenfried self-requested a review September 3, 2024 10:05

leonardehrenfried reviewed Sep 3, 2024

View reviewed changes

src/test/java/org/opentripplanner/updater/trip/moduletests/addition/AddedTest.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Sep 3, 2024

View reviewed changes

src/test/java/org/opentripplanner/updater/trip/moduletests/addition/AddedTest.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Sep 3, 2024

View reviewed changes

src/test/java/org/opentripplanner/updater/trip/moduletests/addition/AddedTest.java Outdated Show resolved Hide resolved