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.

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

Fix validation of running period for NeTEx flexible lines #5007

Merged

Conversation

vpaturet
Copy link
Contributor

@vpaturet vpaturet commented Mar 29, 2023

Summary

As detailed in #5006 , the calculation of the start and end of the transit period is wrong when a flexible line contains both fixed stops and flexible areas.
The construction of TripTimes in this case is necessary since Raptor makes use of flexible lines when they contain at least 2 fixed stops. Moreover these TripTimes are also used to provide flexible trip information in the TransModel API.

This PR relaxes the validation rule of the running period for flexible trips. Flexible trips are identified with Route.getFlexibleLineType() , so this fix covers only flexible lines imported from NeTEx datasets.

Issue

Partially fixes #5006

Unit tests

Documentation

No

@leonardehrenfried
Copy link
Member

I think you want to have trip times so you can look them up in the API, isn't it? That was the reason we added them.

Can you check that the Transmodel API still returns the same values before and after this change?

@t2gran t2gran added this to the 2.3 milestone Mar 29, 2023
@t2gran t2gran added the bug label Mar 29, 2023
@t2gran
Copy link
Member

t2gran commented Mar 29, 2023

This is poor design, reusing TripTimes to pass on information - breaking some core assumptions in TripTimes (purpose is scheduled routing). I think we need to discuss it.

@hannesj
Copy link
Contributor

hannesj commented Mar 29, 2023

The mapping is required for trips containing at least two scheduled fixed stops, as they will be used in RAPTOR routing for intermediate legs. However those might not have a scheduled time on the first stop, which will break the sorting.

This information need to be stored somewhere, so that we can show the time windows for the flex trips in the APIs

@vpaturet
Copy link
Contributor Author

Then obviously if these trips are used in Raptor, they cannot be filtered out at mapping time.
A quick fix, short of a better solution, would be to relax the validation of the running period for flexible trips, using Route.getFlexibleLineType() to identify these trips.
(This does not solve the issue for GTFS-flex nor address the underlying design issue)

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (c805184) 64.08% compared to head (7bd10ac) 64.08%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #5007   +/-   ##
==========================================
  Coverage      64.08%   64.08%           
  Complexity     13593    13593           
==========================================
  Files           1676     1676           
  Lines          66288    66288           
  Branches        7154     7155    +1     
==========================================
+ Hits           42479    42483    +4     
+ Misses         21430    21426    -4     
  Partials        2379     2379           
Impacted Files Coverage Δ
...ithm/raptoradapter/transit/TripPatternForDate.java 72.50% <0.00%> (-1.86%) ⬇️

... and 8 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vpaturet vpaturet changed the title Fix mapping of trip times for NeTEx flexible lines Fix validation of running period for NeTEx flexible lines Mar 30, 2023
@vpaturet vpaturet force-pushed the otp2_fix_flex_trip_times_mapping branch from 7bd10ac to 8f943a6 Compare March 30, 2023 09:04
@vpaturet vpaturet marked this pull request as ready for review March 30, 2023 09:14
@vpaturet vpaturet requested a review from a team as a code owner March 30, 2023 09:14
@t2gran t2gran merged commit 1b1f95a into opentripplanner:dev-2.x Apr 11, 2023
5 checks passed
@t2gran t2gran deleted the otp2_fix_flex_trip_times_mapping branch April 11, 2023 16:05
t2gran pushed a commit that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect TripTimes mapping for NeTEx flexible lines
4 participants