-
Notifications
You must be signed in to change notification settings - Fork 119
feat: validate stair_count, traversal_time and max_slope against pathway_mode #2170
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
Open
vssinghh
wants to merge
4
commits into
MobilityData:master
Choose a base branch
from
vssinghh:feat/stair-count-recommended-for-stairs
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
d450f37
feat: recommend stair_count for stairs pathways
vssinghh ad6f5d5
feat: validate traversal_time and max_slope against pathway_mode
vssinghh b2000b7
feat: recommend length for walkways, fare gates and exit gates
vssinghh 2d1a969
fix: skip the pathway_mode checks when the mode is unrecognized
vssinghh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
123 changes: 123 additions & 0 deletions
123
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/PathwayModeFieldsValidator.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| package org.mobilitydata.gtfsvalidator.validator; | ||
|
|
||
| import static org.mobilitydata.gtfsvalidator.notice.SeverityLevel.WARNING; | ||
|
|
||
| import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice; | ||
| import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice.FileRefs; | ||
| import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator; | ||
| import org.mobilitydata.gtfsvalidator.notice.MissingRecommendedFieldNotice; | ||
| import org.mobilitydata.gtfsvalidator.notice.NoticeContainer; | ||
| import org.mobilitydata.gtfsvalidator.notice.ValidationNotice; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsPathway; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsPathwayMode; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsPathwaySchema; | ||
|
|
||
| /** | ||
| * Validates the pathway fields whose expectations depend on {@code pathway_mode}. | ||
| * | ||
| * <ul> | ||
| * <li>{@code length} is recommended for walkways ({@code pathway_mode=1}), fare gates ({@code | ||
| * pathway_mode=6}) and exit gates ({@code pathway_mode=7}). | ||
| * <li>{@code stair_count} is recommended for stairs ({@code pathway_mode=2}). | ||
| * <li>{@code traversal_time} is recommended for moving sidewalks ({@code pathway_mode=3}), | ||
| * escalators ({@code pathway_mode=4}) and elevators ({@code pathway_mode=5}). | ||
| * <li>{@code max_slope} should only be used with walkways ({@code pathway_mode=1}) and moving | ||
| * sidewalks ({@code pathway_mode=3}). | ||
| * </ul> | ||
| * | ||
| * <p>Generated notices: {@link MissingRecommendedFieldNotice}, {@link | ||
| * IrrelevantMaxSlopeSetForPathwayModeNotice}. | ||
| */ | ||
| @GtfsValidator | ||
| public class PathwayModeFieldsValidator extends SingleEntityValidator<GtfsPathway> { | ||
|
|
||
| @Override | ||
| public void validate(GtfsPathway entity, NoticeContainer noticeContainer) { | ||
| GtfsPathwayMode pathwayMode = entity.pathwayMode(); | ||
|
|
||
| if (recommendsLength(pathwayMode) && !entity.hasLength()) { | ||
| noticeContainer.addValidationNotice( | ||
| new MissingRecommendedFieldNotice( | ||
| GtfsPathway.FILENAME, entity.csvRowNumber(), GtfsPathway.LENGTH_FIELD_NAME)); | ||
| } | ||
|
|
||
| if (pathwayMode == GtfsPathwayMode.STAIRS && !entity.hasStairCount()) { | ||
| noticeContainer.addValidationNotice( | ||
| new MissingRecommendedFieldNotice( | ||
| GtfsPathway.FILENAME, entity.csvRowNumber(), GtfsPathway.STAIR_COUNT_FIELD_NAME)); | ||
| } | ||
|
|
||
| if (recommendsTraversalTime(pathwayMode) && !entity.hasTraversalTime()) { | ||
| noticeContainer.addValidationNotice( | ||
| new MissingRecommendedFieldNotice( | ||
| GtfsPathway.FILENAME, entity.csvRowNumber(), GtfsPathway.TRAVERSAL_TIME_FIELD_NAME)); | ||
| } | ||
|
|
||
| // The spec defines both an empty max_slope and a max_slope of 0 as "no slope", so a zero value | ||
| // carries no more meaning than leaving the field out and is not worth reporting. | ||
| if (!allowsMaxSlope(pathwayMode) && entity.hasMaxSlope() && entity.maxSlope() != 0) { | ||
| noticeContainer.addValidationNotice(new IrrelevantMaxSlopeSetForPathwayModeNotice(entity)); | ||
| } | ||
| } | ||
|
|
||
| private static boolean recommendsLength(GtfsPathwayMode pathwayMode) { | ||
| switch (pathwayMode) { | ||
| case WALKWAY: | ||
| case FARE_GATE: | ||
| case EXIT_GATE: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private static boolean recommendsTraversalTime(GtfsPathwayMode pathwayMode) { | ||
| switch (pathwayMode) { | ||
| case MOVING_SIDEWALK: | ||
| case ESCALATOR: | ||
| case ELEVATOR: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private static boolean allowsMaxSlope(GtfsPathwayMode pathwayMode) { | ||
| switch (pathwayMode) { | ||
| case WALKWAY: | ||
| case MOVING_SIDEWALK: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A pathway that is not a walkway or a moving sidewalk defines `max_slope`. | ||
| * | ||
| * <p>The GTFS specification states that `max_slope` should only be used with walkways | ||
| * (`pathway_mode=1`) and moving sidewalks (`pathway_mode=3`). A `max_slope` of `0` means no slope | ||
| * and is not reported. | ||
| */ | ||
| @GtfsValidationNotice(severity = WARNING, files = @FileRefs({GtfsPathwaySchema.class})) | ||
| static class IrrelevantMaxSlopeSetForPathwayModeNotice extends ValidationNotice { | ||
| /** The row number of the faulty record. */ | ||
| private final int csvRowNumber; | ||
|
|
||
| /** The `pathway_id` of the faulty record. */ | ||
| private final String pathwayId; | ||
|
|
||
| /** The `pathway_mode` of the faulty record. */ | ||
| private final int pathwayMode; | ||
|
|
||
| /** The `max_slope` defined on the faulty record. */ | ||
| private final double maxSlope; | ||
|
|
||
| IrrelevantMaxSlopeSetForPathwayModeNotice(GtfsPathway pathway) { | ||
| this.csvRowNumber = pathway.csvRowNumber(); | ||
| this.pathwayId = pathway.pathwayId(); | ||
| this.pathwayMode = pathway.pathwayMode().getNumber(); | ||
| this.maxSlope = pathway.maxSlope(); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
168 changes: 168 additions & 0 deletions
168
...rc/test/java/org/mobilitydata/gtfsvalidator/validator/PathwayModeFieldsValidatorTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| package org.mobilitydata.gtfsvalidator.validator; | ||
|
|
||
| import static com.google.common.truth.Truth.assertThat; | ||
|
|
||
| import java.util.List; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.JUnit4; | ||
| import org.mobilitydata.gtfsvalidator.notice.MissingRecommendedFieldNotice; | ||
| import org.mobilitydata.gtfsvalidator.notice.NoticeContainer; | ||
| import org.mobilitydata.gtfsvalidator.notice.ValidationNotice; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsPathway; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsPathwayMode; | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| public class PathwayModeFieldsValidatorTest { | ||
|
|
||
| // stair_count is recommended for stairs (pathway_mode=2). | ||
|
|
||
| @Test | ||
| public void stairsWithoutStairCount_yieldsNotice() { | ||
| assertThat(validationNoticesFor(pathway(GtfsPathwayMode.STAIRS).build())) | ||
| .containsExactly(new MissingRecommendedFieldNotice("pathways.txt", 2, "stair_count")); | ||
| } | ||
|
|
||
| @Test | ||
| public void stairsWithStairCount_yieldsNoNotice() { | ||
| assertThat(validationNoticesFor(pathway(GtfsPathwayMode.STAIRS).setStairCount(5).build())) | ||
| .isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void stairsWithNegativeStairCount_yieldsNoNotice() { | ||
| // The spec says a negative stair_count describes walking down, so it is still defined. | ||
| assertThat(validationNoticesFor(pathway(GtfsPathwayMode.STAIRS).setStairCount(-5).build())) | ||
| .isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void nonStairsWithoutStairCount_yieldsNoStairCountNotice() { | ||
| for (GtfsPathwayMode mode : GtfsPathwayMode.values()) { | ||
| if (mode == GtfsPathwayMode.STAIRS) { | ||
| continue; | ||
| } | ||
| assertThat(validationNoticesFor(pathway(mode).setTraversalTime(30).build())) | ||
| .doesNotContain(new MissingRecommendedFieldNotice("pathways.txt", 2, "stair_count")); | ||
| } | ||
| } | ||
|
|
||
| // traversal_time is recommended for moving sidewalks, escalators and elevators. | ||
|
|
||
| @Test | ||
| public void mechanicalPathwaysWithoutTraversalTime_yieldNotice() { | ||
| for (GtfsPathwayMode mode : | ||
| List.of( | ||
| GtfsPathwayMode.MOVING_SIDEWALK, GtfsPathwayMode.ESCALATOR, GtfsPathwayMode.ELEVATOR)) { | ||
| assertThat(validationNoticesFor(pathway(mode).build())) | ||
| .containsExactly(new MissingRecommendedFieldNotice("pathways.txt", 2, "traversal_time")); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void mechanicalPathwaysWithTraversalTime_yieldNoNotice() { | ||
| for (GtfsPathwayMode mode : | ||
| List.of( | ||
| GtfsPathwayMode.MOVING_SIDEWALK, GtfsPathwayMode.ESCALATOR, GtfsPathwayMode.ELEVATOR)) { | ||
| assertThat(validationNoticesFor(pathway(mode).setTraversalTime(45).build())).isEmpty(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void walkwayWithoutTraversalTime_yieldsNoNotice() { | ||
| assertThat(validationNoticesFor(pathway(GtfsPathwayMode.WALKWAY).setLength(12.0).build())) | ||
| .isEmpty(); | ||
| } | ||
|
|
||
| // length is recommended for walkways, fare gates and exit gates. | ||
|
|
||
| @Test | ||
| public void pathwaysRecommendingLengthWithoutLength_yieldNotice() { | ||
| for (GtfsPathwayMode mode : | ||
| List.of(GtfsPathwayMode.WALKWAY, GtfsPathwayMode.FARE_GATE, GtfsPathwayMode.EXIT_GATE)) { | ||
| assertThat(validationNoticesFor(pathway(mode).build())) | ||
| .containsExactly(new MissingRecommendedFieldNotice("pathways.txt", 2, "length")); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void pathwaysRecommendingLengthWithLength_yieldNoNotice() { | ||
| for (GtfsPathwayMode mode : | ||
| List.of(GtfsPathwayMode.WALKWAY, GtfsPathwayMode.FARE_GATE, GtfsPathwayMode.EXIT_GATE)) { | ||
| assertThat(validationNoticesFor(pathway(mode).setLength(12.0).build())).isEmpty(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void zeroLength_yieldsNoNotice() { | ||
| // Unlike max_slope, the spec gives no special meaning to a length of 0, so it counts as | ||
| // defined. | ||
| assertThat(validationNoticesFor(pathway(GtfsPathwayMode.WALKWAY).setLength(0.0).build())) | ||
| .isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void stairsWithoutLength_yieldsNoLengthNotice() { | ||
| assertThat(validationNoticesFor(pathway(GtfsPathwayMode.STAIRS).setStairCount(5).build())) | ||
| .doesNotContain(new MissingRecommendedFieldNotice("pathways.txt", 2, "length")); | ||
| } | ||
|
|
||
| // max_slope should only be used with walkways and moving sidewalks. | ||
|
|
||
| @Test | ||
| public void maxSlopeOnDisallowedMode_yieldsNotice() { | ||
| GtfsPathway entity = | ||
| pathway(GtfsPathwayMode.ELEVATOR).setMaxSlope(0.083).setTraversalTime(30).build(); | ||
| assertThat(validationNoticesFor(entity)) | ||
| .containsExactly( | ||
| new PathwayModeFieldsValidator.IrrelevantMaxSlopeSetForPathwayModeNotice(entity)); | ||
| } | ||
|
|
||
| @Test | ||
| public void maxSlopeOnWalkwayOrMovingSidewalk_yieldsNoNotice() { | ||
| assertThat( | ||
| validationNoticesFor( | ||
| pathway(GtfsPathwayMode.WALKWAY).setMaxSlope(0.083).setLength(12.0).build())) | ||
| .isEmpty(); | ||
| assertThat( | ||
| validationNoticesFor( | ||
| pathway(GtfsPathwayMode.MOVING_SIDEWALK) | ||
| .setMaxSlope(0.083) | ||
| .setTraversalTime(30) | ||
| .build())) | ||
| .isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void zeroMaxSlopeOnDisallowedMode_yieldsNoNotice() { | ||
| // The spec treats an empty max_slope and a max_slope of 0 alike, both meaning no slope. | ||
| GtfsPathway entity = | ||
| pathway(GtfsPathwayMode.ELEVATOR).setMaxSlope(0.0).setTraversalTime(30).build(); | ||
| assertThat(validationNoticesFor(entity)).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void negativeMaxSlopeOnDisallowedMode_yieldsNotice() { | ||
| // A negative slope describes a downward pathway, so it is a real value. | ||
| GtfsPathway entity = | ||
| pathway(GtfsPathwayMode.EXIT_GATE).setMaxSlope(-0.05).setLength(12.0).build(); | ||
| assertThat(validationNoticesFor(entity)) | ||
| .containsExactly( | ||
| new PathwayModeFieldsValidator.IrrelevantMaxSlopeSetForPathwayModeNotice(entity)); | ||
| } | ||
|
|
||
| private static GtfsPathway.Builder pathway(GtfsPathwayMode pathwayMode) { | ||
| return new GtfsPathway.Builder() | ||
| .setCsvRowNumber(2) | ||
| .setPathwayId("pathway1") | ||
| .setFromStopId("stop1") | ||
| .setToStopId("stop2") | ||
| .setPathwayMode(pathwayMode); | ||
| } | ||
|
|
||
| private static List<ValidationNotice> validationNoticesFor(GtfsPathway entity) { | ||
| NoticeContainer noticeContainer = new NoticeContainer(); | ||
| new PathwayModeFieldsValidator().validate(entity, noticeContainer); | ||
| return noticeContainer.getValidationNotices(); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a small risk here if the pathwayMode is illegal (e.g. 9). In that case the validator emits a
UnexpectedEnumValueNoticebefore calling the validate, which is fitting.But then goes on to this code and might emit a
IrrelevantMaxSlopeSetForPathwayModeNotice, which is not really appropriate since we don't really know the mode.I suggest adding a test for UNRECOGNIZED at the top the validate function and just return in that case.
Granted it's small risk (the mode has to be illegal AND the max slope has to be non-zero and the result it just an annoying warning), but it's also easy to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks. You're right, and it's a bit wider than an illegal mode: a missing pathway_mode also arrives as UNRECOGNIZED, so max_slope on its own was enough to trigger the warning, with pathway_mode reported as -1.
Pushed the early return you suggested, plus two tests