feat: Too fast trip travel speed #710

lionel-nj · 2021-02-01T21:10:21Z

closes #517

Please review after #708

Summary:

This PR implements a new validation rule: check trip travel is reasonable.

Expected behavior:

a notice should be generated if at some point the mean speed of a trip (or part or it) exceed 150 km/h

Please make sure these boxes are checked before submitting your pull request - thanks!

Run the unit tests with gradle test to make sure you didn't break anything
Format the title like "feat: -new feature short description-" (PR title must follow the conventional commit specification)
Linked all relevant issues
~~[ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)~~

- implement new validation rule - implement GeoUtil class - draft unit tests - create new notice - update RULES.md

- write new logic rule - additional unit tests - update RULES.md

# Conflicts: # RULES.md

lionel-nj · 2021-02-10T15:40:54Z

This PR relies on a dependency that is not supported by Google, as a result, the implementation of this new rule is postponed until @aababilov evaluates possibilities #708 (comment)

#708

* Only process each trip once * Refactor forEach to simple loop to clarify behavior * Refactor distance variables and method to use double instead of float and int * Refactor utility method signature to remove need for trip object * Cleanup docs - remove references to "error" * Add docs for method of distance calculation * Fix existing tests

# Conflicts: # RULES.md # main/src/main/java/org/mobilitydata/gtfsvalidator/util/GeospatialUtil.java # main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidator.java # main/src/test/java/org/mobilitydata/gtfsvalidator/util/GeospatialUtilTest.java # main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidatorTest.java

…idator

…/gtfs-validator into fix-stop-too-far-from-shape

- draft one more unit test to verify the number of method invocations - make checkStopsWithinTripShape package private

Mockito captures parameters by reference and not value, so we get the final state of the cache, not the state after each method invocation

stop_times.txt should only have stops location_types 0 and 4, so validation for that can be done elsewhere.

This also has the effect of using Euclidean instead of Geodetic buffers, which saves execution time and likely memory, and doesn't seem to result in a level of error that causes problems. So the tradeoff of performance vs. accuracy seems acceptable. See #750 (comment) for details.

…o too-fast-travel

- split validator method - add javadocs

barbeau

Thanks @lionel-nj! Some comments in-line.

RULES.md

...src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidator.java

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TripTravelSpeedValidator.java

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TripTravelSpeedValidatorTest.java

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TripTravelSpeedValidator.java

# Conflicts: # main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidator.java # main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidatorTest.java

- check algorithm on non-timepoints stop times - check algorithm on StopTimeWithArrivalBeforePreviousDepartureTimeNotice case

- handle non-timepoints stop times - handle StopTimeWithArrivalBeforePreviousDepartureTimeNotice situation - rework checkSpeedAlongTrip to take tripId and tripStopTimes as a list - as two parameters - update javadocs

barbeau

Thanks @lionel-nj, looks pretty good! A few more items in-line.

main/src/main/java/org/mobilitydata/gtfsvalidator/notice/FastTravelBetweenStopsNotice.java

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TripTravelSpeedValidator.java

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TripTravelSpeedValidatorTest.java

maximearmstrong

LGTM

- refactor for consistency - beginStopSequence: rework rule logic to no longer maintain a list on integer - rework tests for clarity - additional unit test - improve documentation TODO: finish test refactor + additional unit test for non time points fast and non fast travel

lionel-nj · 2021-02-25T04:29:58Z

@barbeau reading #765 (comment): it seems like I implemented the solution 2 you suggested.

Add checks within separate TripTravelSpeedValidator for valid times (but don't generate notice for invalid times) - Pros are that this ensures the output of FastTravelBetweenStopsNotice is valid and it doesn't add complexity to a single validator class for this and other notices. Cons are duplicating some code for the time checks and another loop through stop_times.txt for this separate validator.

lionel-nj · 2021-02-25T15:44:53Z

We need another two tests where both arrival and departure time aren't provided in between stop times that have arrival and departure times, both for the case where there is and is not too fast travel

Done. I also reworked the tests to ease clarity and reading. @barbeau

barbeau

LGTM, thanks @lionel-nj

barbeau · 2021-02-25T16:12:19Z

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TooFastTravelValidatorTest.java

+  @Test
+  public void fastTravelFarStopsShouldGenerateNotice() {
+    // sample table descriptions available at:
+    // https://gist.github.com/lionel-nj/d21cc8b1519d4f44f9ebd53858cda78d


💯 On the Gist with the table

lionel-nj added 6 commits February 1, 2021 10:12

refactor: validation rule E052 - Stop too far from trip shape

b5bf692

- implement new validation rule - implement GeoUtil class - draft unit tests - create new notice - update RULES.md

fix: failing tests

d9a0b8e

test: additional unit tests

76eaa42

chore: set copyright headers

da87d40

feat: too fast travel

6bc47f2

- write new logic rule - additional unit tests - update RULES.md

Merge branch 'master' into too-fast-travel

8e3b3b7

# Conflicts: # RULES.md

lionel-nj self-assigned this Feb 1, 2021

lionel-nj marked this pull request as draft February 3, 2021 19:26

Merge branch 'master' into too-fast-travel

cf35252

# Conflicts: # RULES.md

barbeau and others added 20 commits February 15, 2021 17:15

Add changes to tests

8857370

use for loop to improve code clarity

3ccdfd6

use double type

cbdc422

fix tests

e63cead

update javadocs

e22e5bb

Avoid extra list and loop for adding notices

d2b1531

Add units to error message

e6b5ca5

refactor: checkStopsWithinTripShape() into StopTooFarFromTripShapeVal…

de2525d

…idator

Merge branch 'fix-stop-too-far-from-shape' of github.com:MobilityData…

7735d6a

…/gtfs-validator into fix-stop-too-far-from-shape

tests: additional unit test

c43ee90

- draft one more unit test to verify the number of method invocations - make checkStopsWithinTripShape package private

Merge branch 'master' into fix-stop-too-far-from-shape

69a5295

tests: additional unit test

2b00710

- draft one more unit test to verify the number of method invocations - make checkStopsWithinTripShape package private

Fix test that checks number of invocations

70213ac

Mockito captures parameters by reference and not value, so we get the final state of the cache, not the state after each method invocation

Remove check for location type

b6c4149

stop_times.txt should only have stops location_types 0 and 4, so validation for that can be done elsewhere.

Remove test for checking location type

6e426f0

stop_times.txt should only have stops location_types 0 and 4, so validation for that can be done elsewhere.

Add time complexity and more documentation

e47ee19

Fix typos

844104c

lionel-nj added 3 commits February 22, 2021 12:15

Merge branch 'master' into too-fast-travel

5be2851

Merge remote-tracking branch 'origin/fix-stop-too-far-from-shape' int…

65899f8

…o too-fast-travel

chore: improve code clarity

506941a

- split validator method - add javadocs

lionel-nj marked this pull request as ready for review February 22, 2021 19:00

lionel-nj requested review from maximearmstrong and barbeau February 22, 2021 19:01

barbeau suggested changes Feb 22, 2021

View reviewed changes

barbeau reviewed Feb 23, 2021

View reviewed changes

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TripTravelSpeedValidator.java Outdated Show resolved Hide resolved

lionel-nj added 4 commits February 23, 2021 10:53

Merge branch 'master' into too-fast-travel

4a54715

# Conflicts: # main/src/main/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidator.java # main/src/test/java/org/mobilitydata/gtfsvalidator/validator/StopTooFarFromTripShapeValidatorTest.java

additional unit tests

3290d78

docs: update

bd450da

remove list from notice constructor

231eaae

barbeau mentioned this pull request Feb 24, 2021

feat: abort on error #765

Closed

4 tasks

lionel-nj added 3 commits February 24, 2021 11:34

additional unit tests

0715d35

- check algorithm on non-timepoints stop times - check algorithm on StopTimeWithArrivalBeforePreviousDepartureTimeNotice case

rework logic rule

fe310d0

- handle non-timepoints stop times - handle StopTimeWithArrivalBeforePreviousDepartureTimeNotice situation - rework checkSpeedAlongTrip to take tripId and tripStopTimes as a list - as two parameters - update javadocs

improve docs

f66c051

lionel-nj requested a review from barbeau February 24, 2021 16:39

barbeau suggested changes Feb 24, 2021

View reviewed changes

maximearmstrong approved these changes Feb 24, 2021

View reviewed changes

test: additional unit test

7f5f860

lionel-nj requested a review from barbeau February 25, 2021 15:45

lionel-nj added 3 commits February 25, 2021 10:45

docs: update RULES.md

1faa76f

Rename classes to reflect original implementation

5412f7a

Merge branch 'master' into too-fast-travel

4faf332

barbeau approved these changes Feb 25, 2021

View reviewed changes

barbeau merged commit 429eb4c into master Feb 25, 2021

barbeau deleted the too-fast-travel branch February 25, 2021 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Too fast trip travel speed #710

feat: Too fast trip travel speed #710

lionel-nj commented Feb 1, 2021 •

edited

lionel-nj commented Feb 10, 2021

barbeau left a comment

barbeau left a comment

maximearmstrong left a comment

lionel-nj commented Feb 25, 2021

lionel-nj commented Feb 25, 2021

barbeau left a comment

barbeau Feb 25, 2021

feat: Too fast trip travel speed #710

feat: Too fast trip travel speed #710

Conversation

lionel-nj commented Feb 1, 2021 • edited

Please review after #708

lionel-nj commented Feb 10, 2021

barbeau left a comment

Choose a reason for hiding this comment

barbeau left a comment

Choose a reason for hiding this comment

maximearmstrong left a comment

Choose a reason for hiding this comment

lionel-nj commented Feb 25, 2021

lionel-nj commented Feb 25, 2021

barbeau left a comment

Choose a reason for hiding this comment

barbeau Feb 25, 2021

Choose a reason for hiding this comment

lionel-nj commented Feb 1, 2021 •

edited