-
Notifications
You must be signed in to change notification settings - Fork 178
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
Trip-to-trip and route-to-route transfers #284
Conversation
@mbta's GTFS provides this data. |
Would it be too much to ask to include the route-to-route one too? https://developers.google.com/transit/gtfs/reference/gtfs-extensions#TripToTripTransfers |
OpenTripPlanner version 2 supports both trip-to-trip and route-to-route, as well as any combination of these and regular stop-to-stop transfers. |
I wouldn't want to explode the scope of this PR and I think this is a good improvement on it's own but following this it would be good to look into in-seats transfers and how they interact with block_ids where trip to trip transfers can be the basis to build upon. So +1 but I see this as the start of more changes. |
I added route-to-route transfers in this PR (feel free to review if it makes sense). I agree that in-seat transfers and block_id should be addressed in a separate PR. |
@scmcca could you describe in this PR what you consider valid combinations, mandatory combinations and invalid combinations? |
@skinkie Correct me if I'm wrong, but I think any combination is allowed as long as the |
A from_trip_id that does not match from_route_id? Similar for to_trip_id / to_route_id. |
Good point. I'll add wording to require they match. |
I think route + trip could be considered redundant, but still valid if they match. |
I think trip to route would be valid if the |
I think the same route in the opposite direction could still be a guaranteed transfer. But I think we need some clear semantics on what can be expected. |
I believe we need to define the "specificity" of these transfer rules. If From the google extension:
The resulting priority for picking the right transfer rule becomes the following:
Samtrafiken/Trafiklab is a producer, our GTFS feeds use the combination |
@Bertware Thanks for the clear summary of the specificity rules! I'm thinking these could go in the description at the top of the
I agree. Would it be suitable to introduce the "Conditionally Forbidden" concept here:
And then use this to forbid |
Editorial to reword Conditionally Forbidden to RFC 2119
It looks like the proposal is stable. Let's open this for a vote. The vote is for adding the following fields in transfers.txt, as outlined in this PR:
As per the specification amendment process, there is at least one producer (Samtrafiken/Trafiklab) and one consumer (OpenTripPlanner) implementing this extension. Voting ends on 2021-10-04 at 23:59:59 UTC. Any remaining feedback is welcomed. Thanks. |
Regarding the conditionally forbidden *_trip_id with *_route_id. If the _route_id would match the _trip_id match, that wouldn't hurt right? My line of thinking: The minimum is stop_id, can be extended to stop_id to route_id, and most specifically stop_id, route_id and trip_id. |
The suggestion to provide either *_trip_id or *_route_id, but not both, came from @Bertware in this comment which seems like a good idea:
However I can see a case for changing this to define that *_trip_id overrides *_route_id when both are defined, as this is what is defined in the original Google proposal:
|
@scmcca I would like to have the reasoning why route_id + trip_id would be bad. I could understand it is redundant. |
To me that seems like implementation detail from Google, not something the spec should care. Agree with @skinkie that redundant information shouldn't necessary be forbidden. I don't think we want to forbid providing |
Can I suggest we make all the added files optional with a note that:
|
+1 Brody Flannigan Transit Data on behalf of Transcollines. You can add Transcollines as a producer, we have been using those fields for quite some time. Details on our GTFS repo. |
@scmcca i think the documentation is still a bit problematic. "If both I think this makes sense for the route_id definition. But I rather have it rephrased for the second (lines 352/353) something like "Only the trip is considered to be referenced." (Someone with proper writing skills please propose something...) |
@skinkie That text is duplicated in the trip fields to reiterate the interaction between I'd be happy to change it, but can you help me out by defining what is problematic about having that text reiterated in the trip fields? |
I think the emphasis with the trip_id fields should be on the combination of fields referring to a trip. |
Is this captured with the proposed:
|
@scmcca to me this gives the feeling that the route_id still may be unrelated, and not a holistic compound field. |
@skinkie How is it a compound field? My understanding as written in the Google proposal is that if both |
I don't know how your datamodel works. But in our data model one trip has one route, similar to how trips.txt is defined. Hence x => (y,x). |
I would say route_id is not related to the transfer rule if trip_id is set, it could even cause misdirection if it is not sufficiently clear that one should not read route_id if trip_id is set, If a transfer rule to a certain trip is created, then that rule says nothing about transfers to that trip's route, it only says something about transfers to that specific trip. Therefore I believe that the route_id should not be set, as it is not related to the transfer rule that is being defined. Since it is redundant, not offering a benefit, and possibly being misleading, I believe it should not be allowed by the spec.
This would work for me too, but I do not understand why one would or should be able to add the route id if it is to be ignored? At best it has no positive or negative effects, in general it will cause files to be larger and slower to parse (albeit likely insignificant). I don't mind if files are a larger or slower to parse, but making the files bigger without conveying any extra data seems like a waste to me. I would like a reasoning or an example as to why it must be allowed/possible. The (always correct) route id can easily be obtained by reading trips.txt. To me, it seems like a trivial thing for a producer to not set a route_id if a trip_id is provided. Am I missing some specific case case? Are there providers already creating feeds that would not be compatible with this specification?
This is why I would not include from_route_id if from_trip_id is set, or to_route_id when to_trip_id is set. This data is already present in the dataset, and adding it once more in the transfers.txt file would in my opinion achieve the same as adding agency_id to stop_times.txt. |
@Bertware I understand the reasoning but for debugging and for data producers, having the route next to the trip can be useful to give context on the trip. |
Small editorial suggestion that doesn't affect the spec: Replacing "in this case" with ", and" makes it more clear that to_trip_id always takes precedence. Just to make it clear that it doesn't just take precedence in the case of trip_id and route_id matching.
Suggestion:
Same for from_trip_id and from_route_id. |
@Bertware I applied the rewording you suggested in the previous comment. As for:
Though I tend to agree that this is better, this was not how the original Google proposal was written. Therefore, those who have implemented this extension already have instances where a |
@scmcca thanks for the example/reasoning, other than this minor remark it looks good to us. +1 From Samtrafiken/Trafiklab |
+1 OpenGeo |
+1 Transit |
+1 @mbta |
+1 Moovit |
The vote ended on 2021-10-04 at 23:59:59 UTC with 5 votes in favor and 0 opposed. As per the Specification Amendment Process, this vote passes! Thanks to all involved. |
@scmcca thanks :-) So now back to our primary key story. |
Thanks @scmcca for leading this PR! |
Trip-to-trip transfers as flagged in #278 to define the primary ID for transfers.txt. This PR contains an extension from Google as previously advocated for by @antrim in #32.
Seeing that this is an extension, it would be good for producers and consumers to provide proof of implementation here in the discussion.
Review and further discussions are welcomed. Thanks!