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

feat: transfers.txt trip=>route reference validation. #1267

Merged

Conversation

bdferris-v2
Copy link
Collaborator

@bdferris-v2 bdferris-v2 commented Oct 11, 2022

Add validation checks for transfers.txt to check that trip, route, and stop references are all consistent. Namely, we add the following checks:

  1. Any stop referenced in transfers.txt should have a stop/platform or station location type.
  2. For a trip + route reference, the route_id should be consistent between transfers.txt and trips.txt.
  3. For a trip + stop reference, the stop should match on of the trip's stop-time entries.

@isabelle-dr
Copy link
Contributor

Would this close #1114?

@bdferris-v2
Copy link
Collaborator Author

@isabelle-dr actually, based on what I'm reading there, I would have thought PR #1208 should have closed that issue.

@isabelle-dr
Copy link
Contributor

Sorry, I meant: would this close #1268?

When we evaluated the impact of adding trip-to-trip and route-to-route transfers, we missed that additional check for trip-to-route pairing. I noticed that as we were evaluating the addition of in-seat transfers, this is why I opened that issue 😊

@bdferris-v2
Copy link
Collaborator Author

@isabelle-dr indeed it would. I added these checks mostly because I wanted to verify some assumptions in the larger discussion about transfer_type validation. But I think these checks are appropriate to add regardless.

@bdferris-v2
Copy link
Collaborator Author

FYI this is ready for review. This can be seen as a pre-req of sorts to the logic being debated in #1266.

@isabelle-dr
Copy link
Contributor

isabelle-dr commented Oct 17, 2022

Thank you for working on this @bdferris-v2 🙏🙏

One question:

For a trip + stop reference, the stop should match one of the trip's stop-time entries

This type of pair isn't specified in the list of possibilities in transfers.txt, so I am wondering if this is something we should be doing in the first place? Do we allow trip-to-stop pairs?

@bdferris-v2
Copy link
Collaborator Author

@isabelle-dr I'm not totally sure I understand your question. When I talk about a "trip + stop reference", I mean the combination of (from_stop_id + from_trip_id) or (to_stop_id + to_trip_id), as opposed to the cross combination of the two. Is that the same sense you were thinking? For this case, it doesn't make sense for from_trip_id to reference a stop in from_stop_id that is never visited by the referenced trip. That's the piece we are checking here.

But maybe you were asking about a case where from_trip_id is specified but to_trip_id is not? (Recall that stop ids are required in both cases). I think that's covered by case #3: One trip_id defined: from_trip_id or to_trip_id. from the spec, yeah?

@isabelle-dr
Copy link
Contributor

Ah, I see. I thought it was the cross combination of the two, thanks for clarifying.

The documentation is updated, I had a look at the acceptance test report, this LGTM! Merging this PR now.
The world of possibilities with transfers is still a bit obscure to me 🔮, so I have not tried this PR with a test dataset.
Does one of you have one? Or a valid production dataset containing these type of transfers that I could play with, so that I am semi-able to explain these rules to the users?

Thanks again 😊

@isabelle-dr isabelle-dr merged commit ba7a83e into MobilityData:master Oct 17, 2022
@bdferris-v2
Copy link
Collaborator Author

pl-mazowieckie-koleje-mazowieckie-km-gtfs-1011 is an interesting feed to look at. It contains trip-to-trip transfers but it specifically uses station references instead of stop references.

@bdferris-v2 bdferris-v2 deleted the issue/1264/transfer_relations branch October 27, 2022 17:24
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