Add search window to travel time API

[hannesj]

Summary

Extracted from #4906

Contains changes required in RAPTOR for extracting durations from stop arrivals.

[codecov]

Codecov Report

Patch coverage: 34.61% and project coverage change: -0.18 ⚠️

Comparison is base (214f70d) 63.09% compared to head (f12811d) 62.92%.

❗ Current head f12811d differs from pull request most recent head 7c45bb1. Consider uploading reports for the commit 7c45bb1 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4911      +/-   ##
=============================================
- Coverage      63.09%   62.92%   -0.18%     
+ Complexity     13407    13277     -130     
=============================================
  Files           1672     1664       -8     
  Lines          66577    66434     -143     
  Branches        7259     7250       -9     
=============================================
- Hits           42009    41801     -208     
- Misses         22168    22251      +83     
+ Partials        2400     2382      -18     

Impacted Files	Coverage Δ
...tripplanner/ext/traveltime/TravelTimeResource.java	0.00% <0.00%> (ø)
...or/rangeraptor/internalapi/RaptorWorkerResult.java	100.00% <ø> (ø)
...angeraptor/multicriteria/McRaptorWorkerResult.java	33.33% <0.00%> (-2.39%)	⬇️
...or/rangeraptor/standard/StdRaptorWorkerResult.java	88.88% <0.00%> (-11.12%)	⬇️
...ripplanner/raptor/service/DefaultStopArrivals.java	28.00% <20.00%> (-2.00%)	⬇️
...ptor/rangeraptor/standard/besttimes/BestTimes.java	82.08% <88.88%> (+0.73%)	⬆️
...er/inspector/vector/VectorTileResponseFactory.java	0.00% <0.00%> (-66.67%)	⬇️
...ntripplanner/inspector/vector/LayerParameters.java	0.00% <0.00%> (-25.00%)	⬇️
...cygraphqlapi/mapping/LegacyGraphQLCauseMapper.java	6.25% <0.00%> (-12.50%)	⬇️
...raphqlapi/mapping/LegacyGraphQLSeverityMapper.java	12.50% <0.00%> (-12.50%)	⬇️
... and 99 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.

[matthiasfeist]

Hi. Just a quick question. I saw that @t2gran added 2 milestones to this PR. does this mean this feature is gonna be shipped with the next version?

[leonardehrenfried]

Unfortunately it doesn't mean that.

@t2gran has just reapplied all labels after the 2.4 release.

AFAIK there is nobody working on this.

[t2gran]

The process we follow is maybe a bit unclear, but a result of "least work" and 95% ok. The process:

1. All PR are tagged with the milestone 2.x (next release)
2. When we release, we move all NOT merged PRs to the next release, and make sure the merged PRs are tagged correct.
3. There are also Parked and Rejected milestones. Parked, means no one is working on it, we do want it - but do not know when we have time to pick it up again. People can Park their milestone or I do it if nothing has happened for 2 years.

If someone show an interest in a PR, then it is also more likely that we fix it.

[matthiasfeist]

Ah OK, thanks for explaining.

I am really interested in this PR and this functionality as it makes OTP usable for my use case. The problem with the current isochrone implementation is that it doesn't give me a good overview of the reachable area in general but really just looks at the precise time that I specify. So the results can vary a lot depending on if I query for 8:00 or 8:10.

I actually checked out the branch by @hannesj to test the new timeWindow parameter and built OTP locally on my machine. Works like a charm for me, so therefore I'm relatively keen to get this functionality into the official release.

I'm unfortunately not a Java programmer but could I maybe help with something in this PR to make it mergeable? I'm not really sure what's missing...

[leonardehrenfried]

There are some Swedish developers working for Skanetrafiken that you might be able to convince to "adopt" this feature.

[matthiasfeist]

Thanks for the tip.

I still wonder what would be missing to be able to merge this PR into the main codebase.
It seems to me that all tests are passing and this feature is just in the sandbox anyways.

If you could give me a point to what's outstanding to be mergeable, I could adopt this PR for sure :)

[slvlirnoff]

Hello @t2gran you commented in the initial PR

Is is possible to split this PR in two:

• One which does not change Raptor (the changes violates the contract for some of the Raptor classes and need to be done in another way), I can maybe fix after the mc2 work is done, but do not have time to spend on it now.
• Improvements without Raptor changes.

Could you outline what you would like to be implemented differently for this one? Maybe I could have a look.

[t2gran]

I toke a quick look and I am unsure if this can be done without changing Raptor. Any changes to Raptor would consume a lot of my time, so I am not so sure. This is a Sandbox feature and we do not allow for it to make changes to Raptor, witch make the code more complicated or degrade the performance. There are other tools like r5 that does this better.

We are working on several changes to Raptor, so it might be easier to fix this when those are done (heuristics for pass-through search and better cost heuristics).

[matthiasfeist]

Hi. After your last comment I had a look at R5 but it's (for me at least) pretty impossible to use. I mean it's not that surprising as it's not meant to be an OpenSource project... They really want to push people in the commercial offering.

One more suggestion to make this sandbox feature a bit more usable:
The routing engine already has a parameter that configures how many trips are returned. In one of the GraphQL APIs it's called numTripPatterns. If the IsoChrone API would expose this instead of an actual timeWindow, this would help already quite a bit.
I think for me it's really only a problem that the current Isochrone code looks at pretty much a single departure at exactly the time given. I don't really care that much if it looks at the 20 ones after the time given or at an exact interval.
Does that make sense?

[leonardehrenfried]

All of what you say makes sense but the people who have the skills to get something like that through code review are pretty rare, and all of them are very busy. So it's not that we don't want to, but a priority thing.

[t2gran]

I think numTripPatterns is not relevant. The search only consider departure-time and arrival-time. There can only be one pareto optimal path between two location with one criteria. The numberOfTransfers is also available, but not sure if the isochrone can display two criteria at once?

Adding the searchWindow to this will increase the usefulness, because you get the best-time within the depature searchWindow, not just if you leave at a given time. For example: departure-time: 12:00, then you board a bus 12:02, arrive at train station 12:20, WAIT 40 min, board train 13:00. If you instead use a searchWindow you take a later bus 12:32 arrive at 12:50, and board the same train. The trip is now 30 faster, just because of the reduction in wait time. To compensate for this, I think the current isochrone uses the raptor search witch remove wait-time. This of cause, have other problems, like being too optimistic.

After fixing the heuristics this become easier to fix.

I disagree about r5. r5 is written by the same people who first wrote OTP. To implement Raptor in OTP I set up r5, and ported the r5 raptor implementation into OTP. It takes fare less time to learn r5, than to implement this in OTP.