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: Validate that locations are reachable if a station has pathways #997

Merged

Conversation

aababilov
Copy link
Collaborator

@aababilov aababilov commented Sep 1, 2021

Fix issue #974, issue #975

Validate that if a station has pathways, then each location is
reachable in both directions: there is a way to get to that location
from some entrance and a way to get from that location to some
entrance.

Notices are reported for platforms, boarding areas and generic nodes
but not for entrances or stations.

Notices are not reported for platforms that have boarding areas since
such platforms may not have incident pathways. Instead, notices are
reported for the boarding areas.

Validate that if a station has pathways, then each location is
reachable in both directions: there is a way to get to that location
from some entrance and a way to get from that location to some
entrance.

Notices are reported for platforms, boarding areas and generic nodes
but not for entrances or stations.

Notices are not reported for platforms that have boarding areas since
such platforms may not have incident pathways. Instead, notices are
reported for the boarding areas.
@isabelle-dr isabelle-dr added this to the v3.0.0 milestone Sep 1, 2021
@isabelle-dr isabelle-dr self-requested a review September 1, 2021 15:06
Copy link
Contributor

@lionel-nj lionel-nj left a comment

Choose a reason for hiding this comment

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

Thanks @aababilov for this contribution! You will fnd a few comments and questions in line. Also could you update NOTICES.md please? We are planning on possibly removing it but waiting for that it is good to keep it up to date.

final Set<String> pathwayEndpoints = new HashSet<>(pathwayTable.byFromStopIdMap().keySet());
pathwayEndpoints.addAll(pathwayTable.byToStopIdMap().keySet());
final Set<String> stationsWithPathways = findStationsWithPathways(pathwayEndpoints);
final Set<String> withEntrances = traversePathways(FROM_ENTRANCES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking for confirmation: this is the list of stop_ids that are accessible from an entrance, right?

private final boolean hasExit;

PathwayUnreachableLocationNotice(GtfsStop location, boolean hasEntrance, boolean hasExit) {
super(SeverityLevel.WARNING);
Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion with @isabelle-dr, we see this notice as an ERROR since in 9,999 cases out of 10,000 having a locked location is not intended. The future concept of CustomValidator(ex Profile) will allow users to downgrade this notice to WARNING level, we could inform users of the process to downgrade this notice when this is implemented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How did you get those numbers? My search in feeds at Google gives 30 violators of that rule out of 4'000 feeds, i.e. 1 of 133 feeds breaks that rule. Such feeds include top priority cities, so I highly recommend to keep treating them as valid. Losing those feeds would have a strong negative user impact.

I do the following trick to mitigate the problem of inaccessible platforms. When a platform is inaccessible, my algorithm treats it as street-accessible. So Google Maps just give the platform number without specifying walking instructions within the station.

Copy link
Contributor

Choose a reason for hiding this comment

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

That trick about inaccessible platforms! 🔥

To clarify what Lionel wrote: In the extreme majority of cases, having an unreachable location in a station is unintentional, although it is present in some top priority cities datasets.

The vision for this validator at MobilityData is:
1- Providing a canonical set of rules & their associated severity level to help users create spec-compliant datasets. This set of rules is as strict as the specification, and it supports one of our mission: support the creation of high-quality GTFS datasets. This is why we're solving discrepancies for the V3.0.0 release.
2- Allowing custom validation sets where a consumer can define what is making a dataset invalid for their app. They will be able to modify severity levels and also have additional rules. (this is the next feature we are planning on working on)

This means that a dataset triggering this notice will be treated as invalid with regards to the specification, and valid with regards to Google Maps.

What are your thoughts on this vision @aababilov?

Copy link
Contributor

@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.

Amazing contribution! Thank you so much Alexei 🙏

Agreed with @lionel-nj on:

  • Update NOTICES.md because it helps users understand the validation report :) We will officially remove it when the users will be able to see this information somewhere else
  • Set the severity as an error to be compliant with the specification:

Each platform (location_type=0 or empty) or boarding area (location_type=4) must be connected to at least one entrance/exit

@aababilov
Copy link
Collaborator Author

I am very glad that you like my code and I am happy to merge it. Thanks for review!

@aababilov aababilov merged commit 13715c8 into MobilityData:master Sep 2, 2021
@isabelle-dr isabelle-dr mentioned this pull request Oct 28, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new rule: no locked platforms new rule: No dangling locations in pathways.txt
3 participants