fix: Check StopTooFarFromTripShape once per trip stoptimes by barbeau · Pull Request #750 · MobilityData/gtfs-validator

barbeau · 2021-02-15T22:19:16Z

Summary:

Change to check StopTooFarFromTripShape once per trip stoptimes, plus a number of other improvements:

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

Closes #747

@lionel-nj Please review and also add the additional unit tests we discussed. Thanks!

Expected behavior:

Only check each trip once, and only check the stop_times.txt for that trip once

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]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
Linked all relevant issues

* 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

lionel-nj

Just one comment in-line

…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

lionel-nj · 2021-02-17T08:50:15Z

@barbeau I drafted an additional unit to verify the number of invocation on checkStopsWithinTripShape but it does not pass although I think that it should. Do you mind taking a look at the two last assertions of StopTooFarFromTripShapeValidatorTest.eachTripShouldOnlyBeProcessedOnce please? I might have missed something either in the testing data or the code implementation.

The detailed error message:

Actual invocations have different arguments:
stopTooFarFromTripShapeValidator.validate(
org.mobilitydata.gtfsvalidator.notice.NoticeContainer@36234e33
);
-> at org.mobilitydata.gtfsvalidator.validator.StopTooFarFromTripShapeValidatorTest.eachTripShouldOnlyBeProcessedOnce(StopTooFarFromTripShapeValidatorTest.java:675)
stopTooFarFromTripShapeValidator.checkStopsWithinTripShape(
"t1",
[org.mobilitydata.gtfsvalidator.table.GtfsStopTime@2af512db, org.mobilitydata.gtfsvalidator.table.GtfsStopTime@66af4359, org.mobilitydata.gtfsvalidator.table.GtfsStopTime@6cb72993],
"shape1",
[org.mobilitydata.gtfsvalidator.table.GtfsShape@269fb3d1, org.mobilitydata.gtfsvalidator.table.GtfsShape@24fe0e13, org.mobilitydata.gtfsvalidator.table.GtfsShape@3857245e],
org.mobilitydata.gtfsvalidator.table.GtfsStopTableContainer@6d797976,
[shape21005, shape21004, shape11002, shape11001, shape11003]

[shape21005, shape21004, shape11002, shape11001, shape11003]

Here, I think that this set should be empty since it is the first call to checkStopsWithinTripShape.

-> at org.mobilitydata.gtfsvalidator.validator.StopTooFarFromTripShapeValidator.validate(StopTooFarFromTripShapeValidator.java:77)
stopTooFarFromTripShapeValidator.checkStopsWithinTripShape(
"t2",
[org.mobilitydata.gtfsvalidator.table.GtfsStopTime@399cf862, org.mobilitydata.gtfsvalidator.table.GtfsStopTime@553640df],
"shape2",
[org.mobilitydata.gtfsvalidator.table.GtfsShape@39a9028f, org.mobilitydata.gtfsvalidator.table.GtfsShape@347a5b6],
org.mobilitydata.gtfsvalidator.table.GtfsStopTableContainer@26e9435d,
[shape21005, shape21004, shape11002, shape11001, shape11003]

[shape21005, shape21004, shape11002, shape11001, shape11003]

Here I think that this set should be [shape11002, shape11001, shape11003] since it's the second (and last) time checkStopsWithinTripShape is called.

To make sure that checkStopsWithinTripShape is only called twice, I checked this assertion (which returns true). I also verified that there are no more interactions on StopTooFarFromTripShapeValidator class (except the execution of validate() method in the test execution). It just seems like the arguments I thought the method would be called with are different from the one the method is actually called with.

    Mockito.verify(underTest, Mockito.times(2))
        .checkStopsWithinTripShape(
            ArgumentMatchers.anyString(),
            ArgumentMatchers.anyList(),
            ArgumentMatchers.anyString(),
            ArgumentMatchers.any(),
            ArgumentMatchers.any(),
            ArgumentMatchers.anySet());

aababilov

Thank you!

aababilov · 2021-02-15T22:41:16Z

    }

    // Create a polyline from the GTFS shapes data
    ShapeFactory.LineStringBuilder lineBuilder = getShapeFactory().lineString();


Is there a method to preallocate the size needed for all shapePoints? Something like lineBuilder.setCapacity(shapePoints.size())

Good idea, but unfortunately no :(. I looked at the source of spatial4j, and it has a comment in the builder class:

// TODO add dimensionality hint method?

Looks like it's backed by an ArrayList, so a lot of re-sizing will happen here :(

https://github.com/locationtech/spatial4j/blob/1f6e2047f0574a430fc711cf2cd5adf141a8bda9/src/main/java/org/locationtech/spatial4j/shape/jts/JtsShapeFactory.java#L312

In good news, in looking at the library source I realized that you can specify a buffered line directly in the lineBuilder, without having to build the line first and then generating the buffer. So that saves memory and execution time for each trip shape.

However, I tested the change and initially got some confusing results. The unit test (using an original shape to line distance of around 140m - the threshold is 100m) passed with the change, but I ran it against MBTA's dataset with the old and new code, and unexpectedly got different results - directly buffering the line in the builder gave me 8 notices, while creating the second instance (original code) gave me 6 notices.

After more poking around in spatial4j code, it looks like the difference is that when buffering within ShapeFactory.LineStringBuilder directly, it always returns a non-geodesic (straight-line) buffer. Conversely, when you first build and then separately buffer the line, it always returns a geodesic buffer (i.e., one that follows the surface of the earth).

The differences between the two are slight, but enough to cause differences in results on real world datasets. I manually measured the distance between one of the stops and the shape in the newly generated notice using Google Earth Pro and it measures around 106 meters.

Another consideration - as you might expect, I noticed that execution time on Euclidean buffers seems to be consistently less than on geodesic buffers. Here are some measurements running on my laptop from within IntelliJ.

Execution time - Euclidean (seconds):

15

11

12

13

11

12

Execution time - Geodesic (seconds):

21

18

17

16

17

17

16

Given what seems to be a decent performance increase in execution time alone (memory performance should be better too, but I didn't measure it), I'm inclined to go with the Euclidean measurements. For our purposes I think the small error doesn't really matter - our intent is to draw attention to potential GTFS errors with stops far from the trip shape, and if we flag one that's a few meters off it shouldn't be problematic. I've made this change in 58718de.

aababilov · 2021-02-17T19:39:44Z

 * Validates: a {@link GtfsStop} is within a distance threshold for a trip shape.
 *
- * <p>Generated notice: {@link StartAndEndDateOutOfOrderNotice}.
+ * <p>Generated notice: {@link StopTooFarFromTripShapeNotice}.


Could you also give time complexity of this algoritm?

I've added notes on time complexity in e47ee19. I actually had to think about this a bit, and hopefully I've expressed it in a way that's understandable. Improvements welcome!

barbeau · 2021-02-17T20:00:49Z

Here, I think that this set should be empty since it is the first call to checkStopsWithinTripShape.
Here I think that this set should be [shape11002, shape11001, shape11003] since it's the second (and last) time checkStopsWithinTripShape is called.

@lionel-nj Your expectations are right, and match actual execution when you set a debugger breakpoint within checkStopsWithinTripShape() to see the Set contents when executing eachTripShouldOnlyBeProcessedOnce(). So it seems something is wrong with the Mockito setup?

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

barbeau · 2021-02-17T21:10:26Z

So it seems something is wrong with the Mockito setup?

So it seems that Mockito captures parameters for each method invocation by reference, so as a result after execution when we check the contents of the cache it's the final contents after all execution finishes, not the contents after each method invocation. I pushed a change in 70213ac that allows us to test each invocation separately without testing the cache, and then have a final cache test with a note in comments on the above. This fixes the test.

lionel-nj · 2021-02-18T14:13:53Z

Wow, good to know! Thanks @barbeau!

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.

See #750 (comment)

lionel-nj

That LGTM @barbeau, thanks!

aababilov · 2021-02-22T23:42:15Z

This still can be significantly optimized because we need to validate each line variant only once (line variant is effectively a trip without time information). However, we can submit it as-is and improve later.

barbeau · 2021-02-23T16:36:13Z

@aababilov I agree - I opened #766 to discuss the line variant optimizations.

barbeau added 2 commits February 15, 2021 17:15

Add changes to tests

8857370

barbeau requested review from aababilov and lionel-nj February 15, 2021 22:19

barbeau changed the title ~~fix: Check StopTooFarFromTripShape once per trip~~ fix: Check StopTooFarFromTripShape once per trip stoptimes Feb 15, 2021

update javadocs

e22e5bb

lionel-nj suggested changes Feb 16, 2021

View reviewed changes

Comment thread main/src/main/java/org/mobilitydata/gtfsvalidator/util/GeospatialUtil.java Outdated

barbeau added 2 commits February 16, 2021 10:24

Avoid extra list and loop for adding notices

d2b1531

Add units to error message

e6b5ca5

barbeau commented Feb 16, 2021

View reviewed changes

Comment thread main/src/main/java/org/mobilitydata/gtfsvalidator/util/GeospatialUtil.java Outdated

lionel-nj added 5 commits February 16, 2021 18:24

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

aababilov requested changes Feb 17, 2021

View reviewed changes

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

barbeau added 6 commits February 18, 2021 12:27

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

Refactor rule-specific constants, format validator code

98cdefd

barbeau mentioned this pull request Feb 19, 2021

Check allowed location_types for stops referenced by stop_times.stop_id #763

Closed

barbeau added 2 commits February 19, 2021 16:18

Update comment based on latest changes to spec

e288e26

See #750 (comment)

Specify WARNING, refactor variable to include units

4c6ff05

barbeau marked this pull request as ready for review February 19, 2021 21:25

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

f1b4733

lionel-nj approved these changes Feb 22, 2021

View reviewed changes

barbeau requested a review from aababilov February 22, 2021 15:05

barbeau mentioned this pull request Feb 22, 2021

feat: Too fast trip travel speed #710

Merged

3 tasks

barbeau merged commit 9d9b25f into master Feb 23, 2021

barbeau deleted the fix-stop-too-far-from-shape branch February 23, 2021 15:26

barbeau mentioned this pull request Feb 23, 2021

Optimize rules by grouping trips into line variants #766

Closed

barbeau mentioned this pull request Feb 23, 2021

fix: Remove Set.of() #767

Merged

4 tasks

lionel-nj mentioned this pull request Feb 23, 2021

CI - SEVERE: Cannot load GTFS feed - error in opening zip file #768

Closed

Uh oh!

Conversation

barbeau commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionel-nj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lionel-nj commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aababilov left a comment

Choose a reason for hiding this comment

Uh oh!

aababilov Feb 15, 2021

Choose a reason for hiding this comment

Uh oh!

barbeau Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

barbeau Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aababilov Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

barbeau Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

barbeau commented Feb 17, 2021

Uh oh!

barbeau commented Feb 17, 2021

Uh oh!

lionel-nj commented Feb 18, 2021

Uh oh!

lionel-nj left a comment

Choose a reason for hiding this comment

Uh oh!

aababilov commented Feb 22, 2021

Uh oh!

barbeau commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

barbeau commented Feb 15, 2021 •

edited

Loading

lionel-nj commented Feb 17, 2021 •

edited

Loading

barbeau Feb 18, 2021 •

edited

Loading