Add routing using frequency trips#3916
Conversation
hannesj
added
!New Feature
There was a problem hiding this comment.
The classes TripFrequencyBoardSearch, TripFrequencyAlightSearch and events inside raptor have dependencies to the OTP internal model, this is not allowed. So, we need to create interfaces for the roles these classes play in Raptor and implement these in the OTP model (TripPatternForDates, TripPattern?, TripTimes).
The Scheduled trip search uses a variant of the Flyweight design pattern, witch is not followed in the Frequency based implementation. The Flyweight pattern avoid creating new objects for each search, instead the Search play the role of the event as well by keeping the last result it finds. But I see that the event also play the role of a TripSchedule, so it is not a clean cut.
We can talk about this tomorrow, and decide on if we need to fix it or not.
- Add methods in TripSchedule to support itinerary mapping - Encapsulate `tripPatternForDates` and `offsets` in TripPatternForDates - Move all frequency based search and events in raptor to its own package - Make the events final - Reduce visibility where appropriate(from public to package local)
t2gran
force-pushed
the
otp2_frequencies
branch
from
9861e99 to
a5f1174
Compare
The `raptor` package in `org.opentripplanner.routing.algorithm` is confusing, because it is NOT Raptor, just the glue between OTP internal model and Raptor.
…pSearch This is needed in the frequency based trips need.
t2gran
force-pushed
the
otp2_frequencies
branch
from
0948ee1 to
bbf940a
Compare
hannesj
force-pushed
the
otp2_frequencies
branch
from
cff8a53 to
4da22ab
Compare
t2gran
left a comment
t2gran
left a comment
There was a problem hiding this comment.
Only the suggested doc update and I think this is ready.
Co-authored-by: Thomas Gran <t2gran@gmail.com>
|
I tried this code with the Mexico City data set and it works, even though you have to set a very, very short searchWindow to get acceptable response times. I guess this is expected, isn't it? |
|
|
oofpez
left a comment
oofpez
left a comment
There was a problem hiding this comment.
Looks good, I've tested this a little bit in multiple cities and I'm getting results comparable with the Frequency-expansion work done previously. I did some of my testing on an earlier version of this code and picked up some bugs but at least the ones I checked now again seem fixed, so I'll keep you up to date if we find anything else. It's possible we'll use this version in an A/B test for our clients to see if we spot any irregularities.
|
|
||
| // Fixup start time due to raptor handling of headways | ||
| Calendar startTime = leg.getStartTime(); | ||
| startTime.setTimeInMillis(startTime.getTimeInMillis() + headwaySeconds * 1000L); |
There was a problem hiding this comment.
Are these coming from Raptor with 'incorrect' start times, or is this a buffer added to ensure the connection can be made?
There was a problem hiding this comment.
I noticed a lot of buffering in the results when testing this code - which does make sense, so I'm not arguing against it, just curious.
There was a problem hiding this comment.
In the raptor timetable, the trip is boarded at the earliest possible time and alighted at the latest possible time, i.e. the driving time between the two stops + the headway, in case it is a non-exact frequency trip. This is to prevent a "too quick" boarding at the destination of the trip, as well as not to favour infrequent trips with short driving times compared to more frequent trips. In this mapper we then push the leg into the arrival end of that time span.
In the new GraphQL API, we are going to split the Leg into multiple GraphQL types, and in the frequency based leg, I want to include two set of times, one earliest and one latest, so that the client can easily communicate the uncertainty of the times to the client.
| this.endOfRunningPeriod = DateMapper.asDateTime( | ||
| localDate, last.getArrivalTime(last.getNumStops() - 1) | ||
| ); | ||
| // TODO: We expect a pattern only containing trips or frequencies, fix ability to merge |
There was a problem hiding this comment.
perhaps we can assert that this is the case and throw a warning on import, if not done already? It might also be good to add this to tests so we can check that a mixture of frequency and non-frequency data behaves as we expect
There was a problem hiding this comment.
Good catch. I will add a check + issue store entry in the graph build. If we have a request, the pattern can be split in two parts, one for frequency based trips and one for schedule-based.
|
I have though about the performance issues with frequency-based trips and raptor and created an issue #3927 for it. We should merge this now and make the 2.1 release, and pick up the performance improvements later. The concept of having time-shiftable-stop-arrivals in Raptor is something that might prove to be useful for other purposes as well. I do not have more time to work on this now, but hopefully we can return to it later. |
|
Dear all, |
5 participants
Summary
This PR adds support for using frequency entries in the RAPTOR routing.
When using a mostly frequency-based network , I recommend adding
searchWindow=0to the query parameters.Issue
closes #3262. Replaces #3394
Unit tests
None added.
Code style
✅
Documentation
This needs to be added
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
[changelog skip]in the title.