Add support for deleted trips & including real-time cancelations in trip search #4759

hannesj · 2023-01-23T19:12:15Z

Summary

GTFS-RT recently added a DELETED real-time state in google/transit#352. Add support for this, as well as refactoring the internals of OTP, so that trips replaced by other patterns are marked as DELETED, while only real cancellations are marked as CANCELED. This also enables us to add a field includeRealtimeCancellations in the Transmodel API

codecov · 2023-01-23T19:23:03Z

Codecov Report

Base: 61.83% // Head: 61.87% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (ea3a29b) compared to base (bf3086d).
Patch coverage: 63.93% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4759      +/-   ##
=============================================
+ Coverage      61.83%   61.87%   +0.04%     
- Complexity     12713    12831     +118     
=============================================
  Files           1612     1619       +7     
  Lines          64614    64958     +344     
  Branches        7019     7078      +59     
=============================================
+ Hits           39953    40196     +243     
- Misses         22410    22481      +71     
- Partials        2251     2281      +30

Impacted Files	Coverage Δ
...pplanner/ext/siri/SiriTimetableSnapshotSource.java	`0.00% <0.00%> (ø)`
...er/ext/transmodelapi/TransmodelGraphQLPlanner.java	`0.00% <ø> (ø)`
...pter/transit/mappers/TripPatternForDateMapper.java	`82.14% <0.00%> (-7.15%)`	⬇️
...tripplanner/routing/stoptimes/StopTimesHelper.java	`73.58% <0.00%> (-1.42%)`	⬇️
...java/org/opentripplanner/model/TripTimeOnDate.java	`21.95% <16.66%> (-2.00%)`	⬇️
...tripplanner/transit/model/timetable/TripTimes.java	`84.36% <75.00%> (-0.16%)`	⬇️
...pplanner/updater/trip/TimetableSnapshotSource.java	`67.55% <85.71%> (+1.68%)`	⬆️
...lanner/ext/transmodelapi/model/plan/TripQuery.java	`99.61% <100.00%> (+0.01%)`	⬆️
...request/RouteRequestTransitDataProviderFilter.java	`94.91% <100.00%> (+0.47%)`	⬆️
...ing/api/request/preference/TransitPreferences.java	`90.19% <100.00%> (-0.13%)`	⬇️
... and 82 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

leonardehrenfried · 2023-01-24T11:21:10Z

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

@@ -291,7 +291,8 @@ public UpdateResult applyTripUpdates(
              tripId,
              serviceDate
            );
-            case CANCELED -> handleCanceledTrip(tripId, serviceDate);
+            case CANCELED -> handleCanceledTrip(tripId, serviceDate, false);
+            case DELETED -> handleCanceledTrip(tripId, serviceDate, true);


We talked about this in the dev meeting today. I'm not a huge fan of passing booleans to methods. I think I have a slight preference for using an enum instead in this case but will leave the choice up to you: the enum also not very pretty.

leonardehrenfried

Could I perhaps ask you to write a test for a GTFS trip update with scheduleRelationship=DELETED?

Bartosz-Kruba

This is from NeTEx/SIRI point of view.

I think REPLACED would be a better name for the new property. We are not really deleting this trip - we are replacing scheduled version of the trip with realtime-based version of the trip. I wonder if it wouldn't be better to have separate field for this property in TripTimes. This is for internal use only. We will never present this information to a client.

Bartosz-Kruba · 2023-01-26T13:01:24Z

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

@@ -31,4 +31,9 @@ public enum RealTimeState {
   * trip pattern of the scheduled trip.
   */
  MODIFIED,
+
+  /**
+   * The trip has been canceled, and should not be visible to the end user


This is not true. It has been replaced, not cancelled.

Well, in GTFS language it is deleted. This means that you should never ever show it to a user.

This definition comes from the GTFS-RT specification, which we use as a base

Updated description

Bartosz-Kruba · 2023-01-26T13:06:14Z

...pentripplanner/routing/algorithm/raptoradapter/transit/mappers/TripPatternForDateMapper.java

@@ -72,7 +71,7 @@ public TripPatternForDate map(Timetable timetable, LocalDate serviceDate) {
      if (!serviceCodesRunning.contains(tripTimes.getServiceCode())) {
        continue;
      }
-      if (tripTimes.getRealTimeState() == RealTimeState.CANCELED) {
+      if (tripTimes.isDeleted()) {


Shouldn't we exclude both CANCELLED and DELETED here?

No, since those will be either filtered or not in the RaptorRoutingRequestTransitData.

Bartosz-Kruba · 2023-01-26T13:08:59Z

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

@@ -213,13 +213,13 @@ public List<String> getHeadsignVias() {
  }

  public PickDrop getPickupType() {
-    return tripTimes.isCanceled() || tripTimes.isCancelledStop(stopIndex)
+    return tripTimes.isCanceledOrDeleted() || tripTimes.isCancelledStop(stopIndex)


Is this right? Deleted does not mean that the pickup is cancelled.

Bartosz-Kruba · 2023-01-26T13:09:31Z

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

      ? PickDrop.CANCELLED
      : tripPattern.getBoardType(stopIndex);
  }

  public PickDrop getDropoffType() {
-    return tripTimes.isCanceled() || tripTimes.isCancelledStop(stopIndex)
+    return tripTimes.isCanceledOrDeleted() || tripTimes.isCancelledStop(stopIndex)


Same problem as with pickup type

hannesj added 4 commits January 23, 2023 18:00

Update GTFS-RT protbuf, handle deleted trips as canceled

ffa67a6

Separate canceled/deleted handling

264f08e

Add possibility to use canceled trips for trip search

154ed71

Update canceled times in buffer

a712376

hannesj added the New Feature label Jan 23, 2023

hannesj added this to the 2.3 milestone Jan 23, 2023

hannesj requested a review from a team as a code owner January 23, 2023 19:12

hannesj requested review from leonardehrenfried and Bartosz-Kruba January 24, 2023 10:29

leonardehrenfried reviewed Jan 24, 2023

View reviewed changes

leonardehrenfried requested changes Jan 24, 2023

View reviewed changes

Bartosz-Kruba reviewed Jan 26, 2023

View reviewed changes

Bartosz-Kruba previously approved these changes Jan 27, 2023

View reviewed changes

Review fixes

ea3a29b

hannesj dismissed Bartosz-Kruba’s stale review via ea3a29b January 30, 2023 13:59

hannesj added the bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR label Jan 31, 2023

hannesj requested review from Bartosz-Kruba and leonardehrenfried January 31, 2023 10:22

Bartosz-Kruba approved these changes Jan 31, 2023

View reviewed changes

leonardehrenfried approved these changes Jan 31, 2023

View reviewed changes

hannesj merged commit afc954a into opentripplanner:dev-2.x Jan 31, 2023

hannesj deleted the otp_deleted_trips branch January 31, 2023 12:11

t2gran pushed a commit that referenced this pull request Jan 31, 2023

Add changelog entry for #4759 [ci skip]

4ff808b

t2gran pushed a commit that referenced this pull request Jan 31, 2023

Bump serialization version id for #4759

a2b0076

hannesj mentioned this pull request Feb 20, 2023

Include cancelled stops when including cancelled trips in trip search #4851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for deleted trips & including real-time cancelations in trip search #4759

Add support for deleted trips & including real-time cancelations in trip search #4759

hannesj commented Jan 23, 2023

codecov bot commented Jan 23, 2023 •

edited

Loading

leonardehrenfried Jan 24, 2023

leonardehrenfried left a comment

Bartosz-Kruba left a comment

Bartosz-Kruba Jan 26, 2023

leonardehrenfried Jan 26, 2023

hannesj Jan 26, 2023

hannesj Jan 30, 2023

Bartosz-Kruba Jan 26, 2023

hannesj Jan 30, 2023

Bartosz-Kruba Jan 26, 2023

Bartosz-Kruba Jan 26, 2023

Add support for deleted trips & including real-time cancelations in trip search #4759

Add support for deleted trips & including real-time cancelations in trip search #4759

Conversation

hannesj commented Jan 23, 2023

Summary

codecov bot commented Jan 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

Bartosz-Kruba left a 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

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

codecov bot commented Jan 23, 2023 •

edited

Loading