Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup trip times - part B #5525

Merged
merged 7 commits into from
Dec 7, 2023
Merged

Cleanup trip times - part B #5525

merged 7 commits into from
Dec 7, 2023

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Nov 22, 2023 •
edited
Loading

Summary

THIS IS PART B, MORE TO COME ...

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 second 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

Second step to fix #5211

Unit tests

Many unit-tests are added and other are updated

Some of the code in ScheduledTripTimeBuilder is commented out, because of problems with FLEX:

    // TODO: Make a separate TripTimes for flex (if a FLEX trip has some regular stops with
    //       scheduled trip times, the flex times could delegate to this class for this subset.
    //       This has the benefit that the delegate can be fed to Raptor for routing.
    //validateNonIncreasingTimes();

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

✅

@t2gran
feature: Add validation to ScheduledTripTimes (not enabled, FLEX fails)
df41296
@t2gran
refactor: Move normalization of trip-times from mapper to builder.
7ff79bf 
This ensures the normalization is performed even if the mapper is no used.
@t2gran
refactor: Use DataValidationException to signal TripTime errors.
1728c29
@t2gran
refactor: Extract TripTimes interface.
ebbe2fd 
This creates a new interface TripTimes and rename the old class to RealTimeTripTimes and
let the ScheduledTripTimes also implement this interface. This will allow us to wait for
an update before an accusal RealTime object is created.
@t2gran t2gran added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog labels Nov 22, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Nov 22, 2023
@t2gran t2gran changed the title Otp2 cleanup trip times part b Cleanup trip times - part B Nov 22, 2023
@t2gran t2gran force-pushed the otp2_cleanup_trip_times_part_B branch from 72e5d7b to ebbe2fd Compare November 22, 2023 14:15
@t2gran t2gran marked this pull request as ready for review November 22, 2023 14:15
@t2gran t2gran requested a review from a team as a code owner November 22, 2023 14:15
@codecov Codecov
Copy link

codecov bot commented Nov 22, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (dd7d0a5) 66.96% compared to head (379a591) 67.21%.
Report is 62 commits behind head on dev-2.x.

Files Patch % Lines
...ner/transit/model/timetable/RealTimeTripTimes.java 85.14% 23 Missing and 7 partials ⚠️
...er/transit/model/timetable/ScheduledTripTimes.java 50.00% 20 Missing ⚠️
...sit/model/timetable/ScheduledTripTimesBuilder.java 88.57% 2 Missing and 2 partials ⚠️
...ner/updater/spi/DataValidationExceptionMapper.java 60.00% 3 Missing and 1 partial ⚠️
...pplanner/updater/trip/TimetableSnapshotSource.java 78.94% 3 Missing and 1 partial ⚠️
...pplanner/ext/siri/SiriTimetableSnapshotSource.java 0.00% 3 Missing ⚠️
...entripplanner/netex/mapping/TripPatternMapper.java 57.14% 2 Missing and 1 partial ⚠️
...der/issue/service/DefaultDataImportIssueStore.java 0.00% 2 Missing ⚠️
...ripplanner/gtfs/GenerateTripPatternsOperation.java 71.42% 2 Missing ⚠️
.../opentripplanner/ext/siri/ModifiedTripBuilder.java 87.50% 1 Missing ⚠️
... and 5 more
Additional details and impacted files 
@@              Coverage Diff              @@
##             dev-2.x    #5525      +/-   ##
=============================================
+ Coverage      66.96%   67.21%   +0.24%     
- Complexity     15862    16088     +226     
=============================================
  Files           1839     1852      +13     
  Lines          70691    70996     +305     
  Branches        7406     7409       +3     
=============================================
+ Hits           47341    47721     +380     
+ Misses         20877    20810      -67     
+ Partials        2473     2465       -8

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

@vpaturet vpaturet self-requested a review November 23, 2023 10:00
leonardehrenfried
leonardehrenfried reviewed Dec 4, 2023
View reviewed changes
src/main/java/org/opentripplanner/model/Timetable.java
@@ -432,12 +430,12 @@ public boolean isValidFor(LocalDate serviceDate) {
// TODO maybe put this is a more appropriate place
public void setServiceCodes(Map<FeedScopedId, Integer> serviceCodes) {
for (TripTimes tt : this.tripTimes) {
tt.setServiceCode(serviceCodes.get(tt.getTrip().getServiceId()));
((RealTimeTripTimes) tt).setServiceCode(serviceCodes.get(tt.getTrip().getServiceId()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cast going to stay or will it be refactored in a later PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be able to get rid of it.

  • In general the updaters should work on RealTimeTripTimes and the API they use should provide them. There is several ways to achieve this - one way is to keep all RT stuff in separate data structures. Then when searching for "a" TripTime we can have a facade witch delegate to both planned and realtime - but there are other options.
  • I think we are getting rid of the service-code (keeping serviceIds for reference). But, I want another calendar implementation witch works better with Raptor.

leonardehrenfried
leonardehrenfried reviewed Dec 4, 2023
View reviewed changes
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now but I have a question about casting.

@t2gran t2gran requested a review from vpaturet December 6, 2023 16:24
@t2gran t2gran merged commit 8a9a7cf into opentripplanner:dev-2.x Dec 7, 2023
5 checks passed
@t2gran t2gran deleted the otp2_cleanup_trip_times_part_B branch December 7, 2023 14:55
t2gran pushed a commit that referenced this pull request Dec 7, 2023
Bump serialization version id for #5525
28c892d
@leonardehrenfried leonardehrenfried mentioned this pull request Dec 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vpaturet vpaturet vpaturet approved these changes

@leonardehrenfried leonardehrenfried leonardehrenfried approved these changes

Assignees
No one assigned
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog Technical Debt
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

java.lang.IllegalArgumentException: This operation is not defined for negative numbers.
3 participants
@t2gran @leonardehrenfried @vpaturet