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 consolidation stop returning original stop ID. #6156

Closed
wants to merge 3 commits into from

Conversation

miklcct
Copy link
Contributor

@miklcct miklcct commented Oct 14, 2024

Summary

This PR has two changes to fix #6032 .

Fix a core issue of Timetable not linking back to the referenced TripPattern

The bug shows from GraphQL in the sense that the stops returned by the pattern are the primary stops, but once the StopTimes are loaded from the timetable, the stops returned are the original, secondary stops.

In the original core code, the Timetable links back to TripPattern which is supposed to create a circular reference as commented. However, it isn't true once the TripPattern is copied. The change to core code makes sure that Timetable instances are never shared across multiple TripPattern instances, and each Timetable instance has a link back to the owning TripPattern.

The sameAs method in TripPattern is updated to work on content equality of the timetable, instead of the equals() method, which can't be overridden because timetable is mutable and is placed in the set, which requires identity equality.

Use the primary stop object in the legs even if the secondary stop names are to be used.

This allows clients to programmatically query the stop data, while showing the secondary stop names to the user.

Unit tests

Added for static timetables. However, real-time updates with consolidated stops aren't tested yet.

Documentation

None.

scheduledTimetable has to be deleted from TripPatternBuilder because a scheduled timetable can no longer be reused under any circumstances, and deduplication is done by content instead of identity
@miklcct miklcct requested a review from a team as a code owner October 14, 2024 13:25
@leonardehrenfried
Copy link
Member

I'm the author of this sandbox feature and I urge you to not use it and fix your data instead.

This was created for a very special deployment under very special circumstances with very specific use cases in mind. It not touching the core code at all was one of the original conditions for having it, so that's why it is the way it is. Also it is left so that it can be very easily ripped out. I will do so when the deployment has moved off it.

Our rules for sandbox features are that the authors have control over them and I don't want to collaborate on it as IMO it should not exist. Again, please fix the data instead.

Some of your changes to the core model might have merit, but changing the core entities is very, very difficult and requires a lot (weeks) of coordination with the core team. If you want to do it, it helps to show up to the dev meeting.

@leonardehrenfried leonardehrenfried marked this pull request as draft October 15, 2024 09:45
@miklcct
Copy link
Contributor Author

miklcct commented Oct 15, 2024

How is it possible to fix the data across two different data feeds w.r.t. to finding legs using nextLegs and previousLegs? I don't think that's possible in the core code.

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Oct 15, 2024

You need to merge your feeds. If you want well integrated results you need well integrated data. It's difficult, I know, but worth it.

@miklcct
Copy link
Contributor Author

miklcct commented Oct 15, 2024

I don't plan to publish the core changes separately as the circular reference issue only appears when TripPatterns are replaced after building the timetable, which only happens when using this sandbox module.

I am going to prioritise making a combined GB-wide feed (GTFS.pro has done that which helped us developing our first prototype, but we have better quality data for National Rail) which has already been in our business plan, so we can disable this feature.

The main use for this is that some Underground and National Rail services use the same physical platforms on the ground, but there are different NapTAN stop codes for Underground and National Rail.

@miklcct miklcct closed this Oct 15, 2024
@t2gran t2gran added this to the Rejected milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants