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

[Clarification] from/to_stop_id & from/to_trip_id description in transfers.txt #455

Merged

Conversation

tzujenchanmbd
Copy link
Collaborator

Issue

Recently we have received requests to change the behavior of the Canonical GTFS Validator regarding transfers.from_stop_id and transfers.to_stop_id (e.g. MobilityData/gtfs-validator#1621), stemming from different interpretations of the current specification.

The current description of from_stop_id is:

Identifies a stop or station where a connection between routes begins. If this field refers to a station, the transfer rule applies to all its child stops. Refering to a station is forbiden for transfer_types 4 and 5.

Based on the description and the discussion in PR#303, it should indicate that from_stop_id and to_stop_id cannot be empty, and it cannot be a "station" (location_type =1) if transfer_type is 4 or 5.

However, some community members may interpret it as:

  • from_stop_id is required for transfer_type 1, 2, and 3; optional for transfer_type 4 and 5 OR
  • from_stop_id is required for transfer_type 1, 2, and 3; forbidden for transfer_type 4 and 5

To prevent such confusion from happening again, we propose the following modification:

Changes in this PR

  • Change presence from Conditionally Required to Required. According to the definition of presence Required - "The field or file must be included in the dataset and contain a valid value for each record", these 2 fields should be Required.
  • Remove the word "forbidden" from description to avoid potential confusion.

+@gcamp - the main contributor of PR#303

gcamp

This comment was marked as outdated.

@gcamp
Copy link
Contributor

gcamp commented May 16, 2024

LGTM on the stop vs station change, however it should still be Conditionally required. For transfer_type 4 and 5, the stop is optional and the main values used in this case are the trip_ids values.

@tzujenchanmbd
Copy link
Collaborator Author

Thanks for the clarification @gcamp !
158ad1d - Changed presence back to Conditionally Required and explicitly stated when is required or optional.

Copy link
Contributor

@gcamp gcamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Copy link
Collaborator

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tzujenchanmbd I made a few editorial edits for consistency with the rest pf the spec:

  • having "Optional" not in bold
  • enum values in code formatting
  • added a formatting change to from_trip_id to reproduce in to_trip_id to fit with the rest of the spec

gtfs/spec/en/reference.md Outdated Show resolved Hide resolved
gtfs/spec/en/reference.md Outdated Show resolved Hide resolved
gtfs/spec/en/reference.md Outdated Show resolved Hide resolved
@isabelle-dr
Copy link
Collaborator

It might be worth seeing if pieces of #363 could also be included in this PR. The initial scope of it was to revert transfer_type=4 and 5 but it also contained good editorial/clarification fixes.

@eliasmbd eliasmbd added GTFS Schedule Issues and Pull Requests that focus on GTFS Schedule Status: Discussion Issues and Pull Requests that are currently being discussed and reviewed by the community. Change: Clarification Revisions of the current specification to improve understanding. labels May 22, 2024
@tzujenchanmbd
Copy link
Collaborator Author

Included editorial suggestions from #363 . Changes from #363 that would potentially change semantics and/or validator behavior are not included in this PR

@@ -625,12 +625,12 @@ For a given ordered pair of arriving trip and departing trip, the transfer with

| Field Name | Type | Presence | Description |
| ------ | ------ | ------ | ------ |
| `from_stop_id` | Foreign ID referencing `stops.stop_id` | **Conditionally Required** | Identifies a stop or station where a connection between routes begins. If this field refers to a station, the transfer rule applies to all its child stops. Refering to a station is forbiden for `transfer_types` 4 and 5. |
| `to_stop_id` | Foreign ID referencing `stops.stop_id` | **Conditionally Required** | Identifies a stop or station where a connection between routes ends. If this field refers to a station, the transfer rule applies to all child stops. Refering to a station is forbiden for `transfer_types` 4 and 5. |
| `from_stop_id` | Foreign ID referencing `stops.stop_id` | **Conditionally Required** | Identifies a stop (`location_type=0`) or a station (`location_type=1`) where a connection between routes begins. If this field refers to a station, the transfer rule applies to all its child stops. It must refer to a stop if `transfer_type` is `4` or `5`.<br><br>Conditionally Required:<br>- **Required** if `transfer_type` is `1`, `2`, or `3`.<br>- Optional if `transfer_type` is `4` or `5`. |
Copy link
Collaborator

@isabelle-dr isabelle-dr Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: In the following statement:

It must refer to a stop if transfer_type is 4 or 5

I would add (location_type=0) after "stop".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identifies a stop (location_type=0) or a station (location_type=1) where a connection between routes begins.

This sentence should already limit to location_type 0 or 1 for this field.

Added "(location_type=0)" after the "stop" - e785501

Copy link
Collaborator

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment, this LGTM.

@tzujenchanmbd
Copy link
Collaborator Author

This PR is solely for clarification purposes and does not alter any original meaning. However, since there are quite a few changes in wording, I am initiating a vote to confirm the descriptions with the community.

Voting ends on 2024-09-17 at 23:59:59 UTC.

@tzujenchanmbd tzujenchanmbd changed the title [Clarification] from/to_stop_id presence and description in transfers.txt [Clarification] from/to_stop_id description in transfers.txt Sep 3, 2024
@tzujenchanmbd tzujenchanmbd changed the title [Clarification] from/to_stop_id description in transfers.txt [Clarification] from/to_stop_id & from/to_trip_id description in transfers.txt Sep 3, 2024
@tzujenchanmbd tzujenchanmbd added Status: Voting Pull Requests where the advocate has called for a vote as described in the changes.md and removed Status: Discussion Issues and Pull Requests that are currently being discussed and reviewed by the community. labels Sep 4, 2024
@gcamp
Copy link
Contributor

gcamp commented Sep 9, 2024

+1 Transit

@westontrillium
Copy link
Contributor

+1 from Trillium

@SteveGladstone
Copy link

+1 Maryland Transit Administration

@skinkie
Copy link
Contributor

skinkie commented Sep 11, 2024

+1 OpenGeo

@tzujenchanmbd
Copy link
Collaborator Author

The vote passed on 2024-09-17 at 23:59:59 UTC.

4 votes in favour and no votes against.

Votes came from:
Transit (@gcamp)
Trillium (@westontrillium)
Maryland Transit Administration (@SteveGladstone)
OpenGeo (@skinkie)

Thank you to everyone who participated!

@tzujenchanmbd tzujenchanmbd merged commit c5bf1d8 into google:master Sep 25, 2024
2 checks passed
@tzujenchanmbd tzujenchanmbd removed the Status: Voting Pull Requests where the advocate has called for a vote as described in the changes.md label Sep 27, 2024
@tzujenchanmbd
Copy link
Collaborator Author

The Canonical GTFS Validator V6.0.0 has fixed the unexpected behavior regarding this clarification. Please refer to the release page: https://github.com/MobilityData/gtfs-validator/releases/tag/v6.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change: Clarification Revisions of the current specification to improve understanding. GTFS Schedule Issues and Pull Requests that focus on GTFS Schedule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants