-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Infer vehicle position's service date for ADDED trip #7179
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.
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
base: dev-2.x
Are you sure you want to change the base?
Infer vehicle position's service date for ADDED trip #7179
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #7179 +/- ##
=============================================
- Coverage 72.17% 72.16% -0.01%
+ Complexity 20878 20876 -2
=============================================
Files 2273 2273
Lines 84437 84440 +3
Branches 8424 8424
=============================================
- Hits 60942 60940 -2
- Misses 20519 20522 +3
- Partials 2976 2978 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...n/src/test/java/org/opentripplanner/updater/vehicle_position/RealtimeVehicleMatcherTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Leonard Ehrenfried <[email protected]>
|
Can you fix the build? |
miklcct
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see test cases for both resolving to today and yesterday.
| // yesterday, today or tomorrow. whichever one has the lowest "distance" to now is guessed to be | ||
| // the service day of the undated vehicle position | ||
| // if this is concerning to you, you should put a start_date in your feed. | ||
| return Stream.of(yesterday, today, tomorrow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a daily journey spans over 24 hours (or close to it and delayed), this distance match may result in matching today's unstarted trip rather than the trip currently in progress. Is this a concern or such feeds are so rare that they should put in a start_date anyway in their feed? (This is just a general comment and I am not requesting a change on this.)
| @@ -373,6 +380,17 @@ void inferServiceDateCloseToMidnight() { | |||
| assertEquals(LocalDate.parse("2022-04-04"), inferredDate); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also add a test case where the vehicle position is given in the morning after daytime routes have typically started, resolving to today rather than yesterday?
|
Also, after some thinking, I believe this is not the correct way to fix the bug and it will cause an incorrect match with a valid feed. Trip Update: Line 165 in f64cf4f
Vehicle position. I think that the correct way to fix this issue is to use the real-time timetable data (which is an overlay on the static data) in guessing the service date to identify the service date it runs. In case, the X12345 only runs today (as it is added by the updater) so it should be today. If the trip ID can be found but there is no service for yesterday, today or tomorrow for this trip ID, the vehicle position should be discarded in my opinion. |
Summary
Proposing to improve handling of added trips in GTFS-rt feeds that don't exist in the static schedule by guessing the service date. This will address an NPE that prevents portions of a trip update feed to be processed entirely.
Issue
Fix #7177
Related: #7008 and #6176
Unit tests
Added
Documentation
N/A
Changelog
The changelog file
is generated from the pull-request title, make sure the title describe the feature or issue fixed.
To exclude the PR from the changelog add the label
+Skip Changelogto the PR.Bumping the serialization version id
Not needed.