Skip to content

Fix TPoint generation for real-time generated patterns - #4787

Merged
hannesj merged 3 commits into
opentripplanner:dev-2.xfrom
entur:otp2_fix_trip_transfer_point
Feb 7, 2023
Merged

Fix TPoint generation for real-time generated patterns#4787
hannesj merged 3 commits into
opentripplanner:dev-2.xfrom
entur:otp2_fix_trip_transfer_point

Conversation

@hannesj

@hannesj hannesj commented Jan 31, 2023

Copy link
Copy Markdown
Contributor

Summary

Currently the generation crashes if the TripTransferPoint#getStopPositionInPattern() is greater than RoutingTripPattern#numberOfStopsInPattern, or it would create a transfer between the wrong stop, if the pattern changed. This changes the handling so that we check the index and compare the stop at that point, and generate a constrained transfer only if it is the same stop, or its sibling, at the specified stop point in pattern.

@hannesj hannesj added the !Bug Apply to issues describing a bug and PRs witch fixes it. label Jan 31, 2023
@hannesj
hannesj requested a review from a team as a code owner January 31, 2023 09:00
@codecov

codecov Bot commented Jan 31, 2023

Copy link
Copy Markdown

Codecov Report

Base: 61.87% // Head: 61.97% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (d9542e3) compared to base (ea3a29b).
Patch coverage: 56.80% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #4787      +/-   ##
=============================================
+ Coverage      61.87%   61.97%   +0.09%     
- Complexity     12831    12915      +84     
=============================================
  Files           1619     1644      +25     
  Lines          64958    65527     +569     
  Branches        7078     7131      +53     
=============================================
+ Hits           40196    40612     +416     
- Misses         22481    22603     +122     
- Partials        2281     2312      +31     
Impacted Files Coverage Δ
...ipplanner/ext/datastore/gs/GsDataSourceModule.java 0.00% <ø> (ø)
...aphqlapi/datafetchers/LegacyGraphQLAgencyImpl.java 26.66% <0.00%> (ø)
...graphqlapi/datafetchers/LegacyGraphQLFeedImpl.java 11.11% <0.00%> (ø)
...phqlapi/datafetchers/LegacyGraphQLPatternImpl.java 13.51% <0.00%> (+0.23%) ⬆️
...qlapi/datafetchers/LegacyGraphQLQueryTypeImpl.java 7.78% <0.00%> (+0.05%) ⬆️
...raphqlapi/datafetchers/LegacyGraphQLRouteImpl.java 19.00% <0.00%> (ø)
...graphqlapi/datafetchers/LegacyGraphQLStopImpl.java 11.11% <0.00%> (ø)
...graphqlapi/datafetchers/LegacyGraphQLTripImpl.java 13.40% <0.00%> (ø)
...a/org/opentripplanner/ext/siri/EntityResolver.java 0.00% <0.00%> (ø)
...opentripplanner/ext/siri/SiriFuzzyTripMatcher.java 12.88% <0.00%> (ø)
... and 231 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.

@leonardehrenfried leonardehrenfried changed the title Fix TPoint generation fo real-time generated patterns. Fix TPoint generation for real-time generated patterns. Jan 31, 2023
@leonardehrenfried leonardehrenfried changed the title Fix TPoint generation for real-time generated patterns. Fix TPoint generation for real-time generated patterns Jan 31, 2023
@hannesj hannesj added the Entur Test This is currently being tested at Entur label Jan 31, 2023
@leonardehrenfried

leonardehrenfried commented Jan 31, 2023

Copy link
Copy Markdown
Member

I checked this out now and it doesn't fix the issue I'm having when using the same transfers from two feeds with the same feed id.

You can find the reproduction case here: https://leonard.io/ibi/hopelink-duplicate-feed-ids.zip (it's all input sources including config in a single zip).

However, I think I made it clear that I'm only too happy to tell IBI that duplicate feed entities from different feeds aren't supported.

Could you look at the example and form an opinion if you still want to fix that case? (Not asking you to do it just if we should support it.)

@t2gran t2gran added this to the 2.3 milestone Jan 31, 2023
@jtorin

jtorin commented Feb 2, 2023

Copy link
Copy Markdown
Contributor

Unfortunately this PR doesn't fix our bug at Skånetrafiken either. The AIOOB exception moves to the processing of the non-realtime RoutingTripPattern, to around the code: scheduledPattern.stopIndex(stopPosInPattern)

Nevertheless: I like the direction the PR takes the code, unless the processing of the entries in two separate batches is adding complexity unnecessarily.

@hannesj

hannesj commented Feb 2, 2023

Copy link
Copy Markdown
Contributor Author

Unfortunately this PR doesn't fix our bug at Skånetrafiken either. The AIOOB exception moves to the processing of the non-realtime RoutingTripPattern, to around the code: scheduledPattern.stopIndex(stopPosInPattern)

If that is producing the issue, then there is something wrong with the org.opentripplanner.model.transfer.TripTransferPoint#stopPositionInPattern. Does the trip come from a GTFS or NeTEx data source?

@jtorin

jtorin commented Feb 2, 2023

Copy link
Copy Markdown
Contributor

If that is producing the issue, then there is something wrong with the org.opentripplanner.model.transfer.TripTransferPoint#stopPositionInPattern. Does the trip come from a GTFS or NeTEx data source?

The trips I see failing is from NeTEx.

@hannesj

hannesj commented Feb 2, 2023

Copy link
Copy Markdown
Contributor Author

Can you add what the fields of the TripTransferPoint are, and how the trip pattern looks like? And the source interchange from NeTEx

@hannesj
hannesj requested review from jtorin and t2gran and removed request for leonardehrenfried February 2, 2023 15:19
@jtorin

jtorin commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

The trips I see failing is from NeTEx.

Just wanted to add that the problems we're seeing seems to be unrelated to this PR.

@hannesj
hannesj merged commit 2de0aaa into opentripplanner:dev-2.x Feb 7, 2023
t2gran pushed a commit that referenced this pull request Feb 7, 2023
@hannesj
hannesj deleted the otp2_fix_trip_transfer_point branch March 7, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. Entur Test This is currently being tested at Entur

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants