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: Implement ShapeToStopMatchingValidator #956

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

aababilov
Copy link
Collaborator

@aababilov aababilov commented Aug 13, 2021

It validates that stops match to shapes properly, mostly focusing on
making sure that the stop sequences for trips that reference the
shape can be properly matched to the shape.

ShapeToStopMatchingValidator replaces StopTooFarFromTripShapeValidator.

Dependency on spatial4j is removed. Now GTFS Validator consistently
uses the light-weight S2 Geometry library.

@aababilov aababilov force-pushed the shape-to-stop-matching branch 2 times, most recently from ed92803 to 535a527 Compare August 13, 2021 06:07
@aababilov
Copy link
Collaborator Author

This depends on #957

@aababilov
Copy link
Collaborator Author

This also depends on #958

@lionel-nj
Copy link
Contributor

Closing and reopening to apply the workflow defined in #953 that aims at checking code formatting.

@lionel-nj lionel-nj closed this Aug 16, 2021
@lionel-nj lionel-nj reopened this Aug 16, 2021
@lionel-nj
Copy link
Contributor

Thanks @aababilov, I am currently reviewing the PR - could you fix the merge conflict please?

It validates that stops match to shapes properly, mostly focusing on
making sure that the stop sequences for trips that reference the
shape can be properly matched to the shape.

ShapeToStopMatchingValidator replaces StopTooFarFromTripShapeValidator.

Dependency on spatial4j is removed. Now GTFS Validator upstream can
be compiled at Google.
@aababilov
Copy link
Collaborator Author

All fixed and ready for review!

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.

Did not finish the review yet, but here are a few first comments.

@lionel-nj lionel-nj mentioned this pull request Aug 18, 2021
7 tasks
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.

Another set of comments

StopUtil.getStopOrParentLatLng(stopTable, stopTime.stopId()).toPoint(),
stopTime.shapeDistTraveled(),
stopTime,
stationSize.equals(StationSize.LARGE) && firstOrLastStop));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why considering firstOrLastStop here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agency shapes often do not extend till the very end of the track, especially for train stations. Although this is a data issue that agencies should fix, we would like to be more tolerant and not drop such shapes completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification!

@aababilov
Copy link
Collaborator Author

Thanks for review! PTAL.

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 working on that. LGTM.

@lionel-nj lionel-nj merged commit a65ddaf into MobilityData:master Aug 25, 2021
@isabelle-dr
Copy link
Contributor

@aababilov great work! Thank you for this contribution!

Sorry to be late at the party but could you please update RULES.md with those new rules?

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.

3 participants