OTP2 no longer crashes on invalid GTFS stop time sequences

[vesameskanen]

Summary

Code which filters bad stop time sequences now correctly replaces the invalid stop time list with empty list.

Issue

fixes #4114

[codecov-commenter]

Codecov Report

Merging #4205 (0a1c113) into dev-2.x (aa104f7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             dev-2.x    #4205      +/-   ##
=============================================
+ Coverage      48.94%   48.95%   +0.01%     
- Complexity     10042    10048       +6     
=============================================
  Files           1376     1376              
  Lines          56540    56540              
  Branches        6589     6589              
=============================================
+ Hits           27671    27680       +9     
+ Misses         26839    26833       -6     
+ Partials        2030     2027       -3     

Impacted Files	Coverage Δ
...nner/gtfs/RepairStopTimesForEachTripOperation.java	71.79% <100.00%> (ø)
...ripplanner/model/transfer/ConstrainedTransfer.java	80.00% <0.00%> (-3.34%)	⬇️
...entripplanner/graph_builder/module/GtfsModule.java	68.86% <0.00%> (+0.59%)	⬆️
...g/opentripplanner/api/parameter/QualifiedMode.java	68.42% <0.00%> (+2.63%)	⬆️
...aph_builder/module/OsmBoardingLocationsModule.java	71.69% <0.00%> (+3.77%)	⬆️
...ripplanner/gtfs/GenerateTripPatternsOperation.java	83.56% <0.00%> (+5.47%)	⬆️
...ipplanner/graph_builder/issues/TripDegenerate.java	100.00% <0.00%> (+100.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa104f7...0a1c113. Read the comment docs.

[leonardehrenfried]

But if those stop time sequences are invalid, should OTP not crash?

I think I agree with @hannesj that we should not try to fix invalid trips but rather filter them out and add an issue to the issue store.

[leonardehrenfried]

Having said that I don't think we have a good definition what this "repairing" actually means as it's pretty arbitrary and just guesswork to get something into the graph. Do you agree?

[vesameskanen]

Invalid stoptimes seem to be common in some datasets. If we don't filter them away we can't use the data at all. Filtering is not repairing. After the code change, bad trip is erased completely which is the right action.

[leonardehrenfried]

I just re-read the code around the diff and you're right. It makes no sense trying to interpolate stop times where we have decided that we are going to throw them away. An issue is also already added to the store so you can see which ones are removed.

[leonardehrenfried]

I would feel a bit happier if we had tests for this module but I think the time is better spent adding them after the transit model refactoring.