Cleanup trip times - Part A #5437

t2gran · 2023-10-17T14:44:45Z

Summary

This clean up the TripTimes and split Scheduled trip-times and RealTime trip-times into different classes inheriting the same interface. This allow us to make the ScheduledTripTimes become immutable and freeze it after the graph is build.

This fixes a minor bug - it is no longer possible to create invalid ScheduledTripTimes - even with broken GTFS data (there was an error in the stop times interpolation).

This PR will validate the first departure-time and the last arrival-time to be within -12 hours and 10 days. If someone has a pattern witch last for more than 10 days we can increase the max limit. The min limit is set to -12 hours to account for time-shifting between time-zones. So, even if a feed can only have positive times, all feeds may not operate in the same time-zone.

This is the first refactoring with respect to trip-times. I will follow up this with more PRs. There are now unused code in these classes - these are left as is intentionally. This refactoring introduce a new interface for TripTimes - I have not changed all places to use this interface yet. Also, when the RealTimeTripTimes is in place, then first is the time to remove unused code.

Issue

First step to fix #5211

Unit tests

Some tests are disabled, since they uses invalid TripTimes and started failing when the validation of trip-times where added. Help to fix these test would be appreciated.

Many unit-tests are added and other are updated

Documentation

Does not apply, JavaDoc is updated.

Changelog

This is for most part a refactoring, but the result also fixes a minor bug #5211.

Bumping the serialization version id

✅

codecov · 2023-10-17T14:55:15Z

Codecov Report

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

Comparison is base (0a0d8b0) 66.95% compared to head (dd7d0a5) 66.96%.
Report is 16 commits behind head on dev-2.x.

Files	Patch %	Lines
...er/transit/model/timetable/ScheduledTripTimes.java	55.88%	29 Missing and 1 partial ⚠️
...planner/routing/service/DefaultRoutingService.java	36.84%	11 Missing and 1 partial ⚠️
.../timetable/StopTimeToScheduledTripTimesMapper.java	83.33%	11 Missing and 1 partial ⚠️
...tripplanner/transit/model/timetable/TripTimes.java	82.08%	11 Missing and 1 partial ⚠️
...nner/transit/model/framework/DeduplicatorNoop.java	11.11%	8 Missing ⚠️
...java/org/opentripplanner/raptor/RaptorService.java	41.66%	6 Missing and 1 partial ⚠️
...entripplanner/raptor/api/request/SearchParams.java	0.00%	4 Missing ⚠️
...uting/algorithm/mapping/RoutingResponseMapper.java	78.94%	2 Missing and 2 partials ⚠️
...tor/standard/MinTravelDurationRoutingStrategy.java	0.00%	2 Missing ⚠️
...pplanner/ext/siri/SiriTimetableSnapshotSource.java	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5437      +/-   ##
=============================================
+ Coverage      66.95%   66.96%   +0.01%     
- Complexity     15794    15862      +68     
=============================================
  Files           1832     1839       +7     
  Lines          70540    70691     +151     
  Branches        7410     7406       -4     
=============================================
+ Hits           47229    47341     +112     
- Misses         20840    20877      +37     
- Partials        2471     2473       +2

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

leonardehrenfried · 2023-10-18T13:10:25Z

I will take a look at the failing unit tests.

leonardehrenfried · 2023-10-18T13:28:26Z

Some of the tests are fixed by this commit: leonardehrenfried@fb2c0bb

There is another flex integration test that fails with this PR and I would like to talk about it in the dev meeting.

leonardehrenfried · 2023-10-19T09:08:43Z

I've investigated why the flex integration test fails: it's because of a particularity of the current implementation of ScheduledDeviatedTrips. These are trips which both have scheduled stops and flex areas with windows.

Consider the following trip:

trip_id	stop_sequence	stop_id	arrival_time	departure_time	pickup_type	drop_off_type	start_pickup_drop_off_window	end_pickup_drop_off_window
4d838cf4-d44d-4e08-a364-f22c34a8c89e	0	yz85	7:30:00	7:30:00
4d838cf4-d44d-4e08-a364-f22c34a8c89e	1	zone_1			2	2	7:30:00	8:00:00
4d838cf4-d44d-4e08-a364-f22c34a8c89e	2	zone_1			2	2	7:30:00	8:00:00
4d838cf4-d44d-4e08-a364-f22c34a8c89e	3	cujv	8:00:00	8:00:00

Before this PR the following happened: in addition to creating a flex trip, we would also create a scheduled trip where the flex parts were removed. This means that the regular transit routing engine would also use this trip, but without the flex trips.

It seems that this PR removed this behaviour. We need to discuss if that is desirable.

Bartosz-Kruba · 2023-11-10T13:48:45Z

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

+   */
+  public void validateNonIncreasingTimes() {
+    final int nStops = arrivalTimes.length;
+    int prevDep = -9_999_999;


Can't we have this value as a constant?

Yes, I will fix it when doing the final cleanup of trip-times, but first I need to cleanup the usage - leaving the internals as is. Se my next comment and the PR summary.

Bartosz-Kruba · 2023-11-10T13:53:17Z

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

+   * increasing. The first stop arrival time and the last stops depature time is NOT checked -
+   * these should be ignored by raptor.
+   */
+  private void validateNonIncreasingTimes() {


Looks like this could be a common logic between RealtimeTripTimes and ScheduledTripTimes

Yes, I am not done with the refactoring - there is a lot of duplication and I have intentionally not made any changes here yet. I am refactoring the code using TripTimes first, then when I am done I know what code should be deleted, shared and kept in RT and Scheduled TripTimes - so for know ignore the duplication.

This decouple TripTimes from StopTime and make the code more readable. For example `new TripTimes(tt)`` is replaced with `tt.copyOfScheduledTimes()``.

The need to create a Deduplicator to unit test is make the unit-test setup more messy than it needs to, and it introduces a unknown into the test equation - something that is not under-test. Signed-off-by: Thomas Gran <[email protected]> # Conflicts: # src/test/java/org/opentripplanner/model/plan/legreference/ScheduledTransitLegReferenceTest.java

…evant

t2gran · 2023-11-17T00:25:15Z

I have rebase this PR and removed the code that caused the problems with the ScheduledDeviatedTrips, I would love to merge this to avoid conflicts, then I can continue. I will do this in small steps.

leonardehrenfried · 2023-11-20T11:56:21Z

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

+  ScheduledTripTimes(ScheduledTripTimesBuilder builder) {
+    this.timeShift = builder.timeShift;
+    this.serviceCode = builder.serviceCode;
+    this.arrivalTimes = builder.arrivalTimes;
+    this.departureTimes = builder.departureTimes;
+    this.timepoints = builder.timepoints;
+    this.trip = builder.trip;
+    this.pickupBookingInfos = builder.pickupBookingInfos;
+    this.dropOffBookingInfos = builder.dropOffBookingInfos;
+    this.headsigns = builder.headsigns;
+    this.headsignVias = builder.headsignVias;
+    this.originalGtfsStopSequence = builder.originalGtfsStopSequence;
+  }


I think it would be a good idea to add some invariant checks here. Or are there none?

Yes, that is the plan - and by splitting TR and schedules times the checks can differ.

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

leonardehrenfried · 2023-11-20T12:08:25Z

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

-    final Trip trip,
-    final Collection<StopTime> stopTimes,
-    final Deduplicator deduplicator
+  private TripTimes(final TripTimes original, int timeShiftDelta) {


This constructor is unused.

leonardehrenfried · 2023-11-20T12:11:29Z

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

-    this.occupancyStatus = object.occupancyStatus;
+  }
+
+  public static TripTimes of(ScheduledTripTimes scheduledTripTimes) {


This is unused.

leonardehrenfried · 2023-11-20T12:14:42Z

I have reviewed and while I have questions about what the final design will be, I think it's going into the right direction. There is still quite a bit of mutability in the TripTimes class but my guess is that you will be working on this in upcoming PRs.

leonardehrenfried

I have a few suggestions but I recognize that you are half-done so I'm happy to approve. I would be interested to hear in the dev meeting what the final design will be.

t2gran · 2023-11-21T15:35:53Z

I have a few suggestions but I recognize that you are half-done so I'm happy to approve. I would be interested to hear in the dev meeting what the final

Pleace bring it up on the developer meeting, and I can go through the design a bit. I think the target is described in #4002, but to see that this is a step in that direction is maybe hard to see. But, the main thing about this PR is that TripTimes are today different for RT and Scheduled - there is switches inside the code to handle this using the same type. This of course make the code unessessary complicated. The plan is also to deal with this in separate classes. The trip-times share one thing and that is the usage in Routing and API, but this can be fixed by an interface (introduced in this PR). Then the implementation is/should be different (one of many: scheduled trip times are optimised for deduplication, RT are not). Trip-times are one of the most important classes in OTP and being compleatly DRY here is less important. I think that it will be pretty DRY in the end, but it is not were I want to start. Encapsulation and separation of concern is the most important thing first.

…es_part_A

t2gran · 2023-11-21T15:53:18Z

Sorry for the dismissed reviews, I have only applied the spelling fixes and merged in dev-2.x. There were some conflict so I think the merged would dismiss the reviews anyway - hence including the spelling. Since you both approved, I will wait until tomorrow and then merge it.

t2gran added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Oct 17, 2023

t2gran added this to the 2.5 (next release) milestone Oct 17, 2023

t2gran requested a review from a team as a code owner October 17, 2023 14:44

Bartosz-Kruba reviewed Nov 10, 2023

View reviewed changes

t2gran marked this pull request as draft November 14, 2023 00:06

t2gran added 10 commits November 16, 2023 18:47

doc: Move JavaDoc to getters.

df62197

refactor: Remove duplicated code in stopRealTimeStates setter/getter.

d046a8f

doc: Do not repeat scope in TripTimes JavaDoc.

dc82ddf

refactor: Extract ScheduledTripTimes from TripTimes

c5b3965

refactor: Extract mapping and builder code from ScheduledTripTimes

3bbd17d

refactor: Replace TripTimes copy constructors with factory methods

66a3066

This decouple TripTimes from StopTime and make the code more readable. For example `new TripTimes(tt)`` is replaced with `tt.copyOfScheduledTimes()``.

refactor: Make ScheduledTripTimes immutable

620cadf

refactor: Add equals and hashCode to ScheduledTripTimes

81f9532

refactor: Reorder fields and methods from most to least important/rel…

8297049

…evant

t2gran force-pushed the otp2_cleanup_trip_times_part_A branch from 207bb3f to 8297049 Compare November 17, 2023 00:13

t2gran marked this pull request as ready for review November 17, 2023 00:14

t2gran requested review from Bartosz-Kruba and leonardehrenfried November 17, 2023 11:56

leonardehrenfried reviewed Nov 20, 2023

View reviewed changes

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

leonardehrenfried reviewed Nov 20, 2023

View reviewed changes

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

leonardehrenfried reviewed Nov 20, 2023

View reviewed changes

leonardehrenfried previously approved these changes Nov 20, 2023

View reviewed changes

Bartosz-Kruba previously approved these changes Nov 21, 2023

View reviewed changes

review: Apply spelling fixes to doc.

90929bb

t2gran dismissed stale reviews from Bartosz-Kruba and leonardehrenfried via 90929bb November 21, 2023 15:42

Merge remote-tracking branch 'otp/dev-2.x' into otp2_cleanup_trip_tim…

dd7d0a5

…es_part_A

t2gran requested review from Bartosz-Kruba and leonardehrenfried November 21, 2023 15:49

leonardehrenfried approved these changes Nov 22, 2023

View reviewed changes

t2gran merged commit 9d0bf79 into opentripplanner:dev-2.x Nov 22, 2023
5 checks passed

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

Add changelog entry for #5437 [ci skip]

1f512b4

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

Bump serialization version id for #5437

65a0cbd

t2gran deleted the otp2_cleanup_trip_times_part_A branch December 7, 2023 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup trip times - Part A #5437

Cleanup trip times - Part A #5437

t2gran commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

leonardehrenfried commented Oct 18, 2023

leonardehrenfried commented Oct 18, 2023 •

edited

Loading

leonardehrenfried commented Oct 19, 2023

Bartosz-Kruba Nov 10, 2023

t2gran Nov 17, 2023

Bartosz-Kruba Nov 10, 2023

t2gran Nov 17, 2023

t2gran commented Nov 17, 2023

leonardehrenfried Nov 20, 2023

t2gran Nov 21, 2023

leonardehrenfried Nov 20, 2023

leonardehrenfried Nov 20, 2023

leonardehrenfried commented Nov 20, 2023

leonardehrenfried left a comment

t2gran commented Nov 21, 2023 •

edited

Loading

t2gran commented Nov 21, 2023

Cleanup trip times - Part A #5437

Cleanup trip times - Part A #5437

Conversation

t2gran commented Oct 17, 2023 • edited Loading

Summary

Issue

Unit tests

Documentation

Changelog

Bumping the serialization version id

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

leonardehrenfried commented Oct 18, 2023

leonardehrenfried commented Oct 18, 2023 • edited Loading

leonardehrenfried commented Oct 19, 2023

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

t2gran commented Nov 17, 2023

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

leonardehrenfried commented Nov 20, 2023

leonardehrenfried left a comment

Choose a reason for hiding this comment

t2gran commented Nov 21, 2023 • edited Loading

t2gran commented Nov 21, 2023

t2gran commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

leonardehrenfried commented Oct 18, 2023 •

edited

Loading

t2gran commented Nov 21, 2023 •

edited

Loading