-
Notifications
You must be signed in to change notification settings - Fork 100
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: Check that vehicles do not travel too fast #944
Conversation
Here I am publishing the code that we use at Google. It was reviewed by Google engineers and its logic was used by us for years. I am not using the SchemaExport annotation because I would have to create constructors with 15 parameters. This is error-prone since it is easy to pass params in wrong order. I think we need another way to export the schema. |
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.
Thanks @aabilov for working on that! Just one thing in the comments, otherwise this LGTM @barbeau, @maximearmstrong thoughts?
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java
Show resolved
Hide resolved
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.
Thanks @aababilov for this contribution - great work! 💯
Some comments in-line.
@lionel-nj I suggest highlighting the change of this rule in the release notes for the next release to give people a heads up that the thresholds are changing and that the old notice now maps to two new notices.
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidator.java
Show resolved
Hide resolved
#### FastTravelBetweenFarStopsNotice | ||
|
||
A transit vehicle moves too fast between far consecutive stops (more than in 10 km apart). | ||
This normally indicates a more serious problem than too fast travel between consecutive stops. |
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.
If this is a "more serious problem", should it be elevated to an error instead of warning?
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.
I do not want to reject 225 feeds that have this problem, so I would like to keep it as warning.
By the way, can you run this validator on several thousands feeds that you have in MobilityData?
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.
Makes sense.
@lionel-nj would know more about the state of running against the large feed database. Related, I noticed that even the end-to-end-100 job is failing to complete on recent commits - https://github.com/MobilityData/gtfs-validator/runs/3246259762.
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.
I see, the Validator was out of memory. May we give it 3 GiB? I think it is usually enough.
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.
Validator was out of memory because se-trafiklab reports trains using extended vehicle type (100, 101, 102 etc). The upstream validator does not support extended vehicle types - it shows a warning for unknown enum value. StopTimeTravelSpeedValidator uses 50 km/h threshold for "unknown" vehicle types which is too low for trains, so GTFS Validator produces too many warnings for "fast travel" and gets out of memory.
Mitigation. Use 200 km/h threshold for unknown vehicle types: #946
Solution. Add support for extended route types. At Google. we convert them to basic route types.
/**
* Converts GTFS route type to a narrow range of basic types.
*
* <p>Google supports extended route types (Hierarchical Vehicle Type, HVT) that are not a part of
* GTFS standard but they are widely used. This function converts HVT to basic route types.
*
* <p>Google also supports non-standard route types (intercity bus and commuter train) that are
* also converted to basic ones.
*/
public static BasicType toBasicType(GtfsRouteType routeType) {
switch (routeType) {
case LIGHT_RAIL:
return BasicType.LIGHT_RAIL;
case SUBWAY:
return BasicType.SUBWAY;
case RAIL:
case COMMUTER_TRAIN:
return BasicType.RAIL;
case INTERCITY_BUS:
case BUS:
case HVT_COMMUNAL_TAXI:
return BasicType.BUS;
case FERRY:
return BasicType.FERRY;
case CABLE_TRAM:
return BasicType.CABLE_TRAM;
case AERIAL_LIFT:
return BasicType.AERIAL_LIFT;
case FUNICULAR:
return BasicType.FUNICULAR;
case TROLLEYBUS:
return BasicType.TROLLEYBUS;
case MONORAIL:
case HVT_MONORAIL:
return BasicType.MONORAIL;
case HORSE_CARRIAGE:
case HVT_HORSE_DRAWN_CARRIAGE:
return BasicType.HORSE_CARRIAGE;
default:
break;
}
// Support for Hierarchical Vehicle Types.
switch (routeType.getNumber() / 100) {
case 1:
return BasicType.RAIL;
case 2:
case 7:
return BasicType.BUS;
case 4:
return BasicType.SUBWAY;
case 8:
return BasicType.TROLLEYBUS;
case 9:
return BasicType.LIGHT_RAIL;
case 10:
case 12:
return BasicType.FERRY;
case 13:
return BasicType.AERIAL_LIFT;
case 14:
return BasicType.FUNICULAR;
default:
return BasicType.MISCELLANEOUS;
}
}
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.
By the way, can you run this validator on several thousands feeds that you have in MobilityData?
The process is under construction: #848 & #933. We are considering using Kubernetes to run the validator on all datasets available on the mobility archives (1300+).
I see, the Validator was out of memory. May we give it 3 GiB? I think it is usually enough.
Yes, see #947.
...src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTimeTravelSpeedValidatorTest.java
Show resolved
Hide resolved
Thank you for this contribution @aababilov! 🙏 |
I got a merge conflict for One of the goals of my validator architecture was to avoid modifying the same file when you add a validation since this leads to merge conflicts. Now Let's find a mechanism to generate those files instead of maintaining them manually. |
The new StopTimeTravelSpeedValidator was implemented at used at Google for years. It replaces TooFastTravelValidator. The new validator has the following differences from the old one. * Use Google S2 Geometry library instead of Spatial4J, so it can be built at Google * Check both the speed between consequent stops as well as the speed between far stops * Each line variant is validated only once. This is a major speed up because one line variant typically runs multiple trips. * Different route types have different speed thresholds, so a train may run much faster than a bus.
Thanks for review! |
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.
LGTM
My pleasure! |
The new StopTimeTravelSpeedValidator was implemented at used at Google
for years. It replaces TooFastTravelValidator.
The new validator has the following differences from the old one.
because one line variant typically runs multiple trips.
run much faster than a bus.