Extend stop area relation linking to include bus stop and platform nodes by vesameskanen · Pull Request #5319 · opentripplanner/OpenTripPlanner

vesameskanen · 2023-08-23T11:57:16Z

Summary

Stop_area relation processing got stricter rules when PR Bug fixes in stop area relation processing by vesameskanen · Pull Request #5166 · opentripplanner/OpenTripPlanner got merged. Before that, all member nodes got linked, sometimes through different levels or layers.

This PR relaxes linking restrictions back a bit so that nodes tagged with highway=bus_stop and public_transport=platform are accepted as platform linking points. At least Finnish OSM data contains many platforms where the stop ref tag is attached to such nodes. If the node is isolated, it gets removed and ref tag linking breaks.

An example:

https://www.openstreetmap.org/node/1009636736#map=19/60.28262/24.97928&layers=D

Unit tests

New unit test ensures that relation member tageed as highway=bus_top gets linked.

Documentation

StopAreas.md document updated

It seems that isolated nodes tagged with highway=bus_stop or public_transport=platform are sometimes used for linking transit stops with OSM street network using ref tagging. Such isolated nodes must be linked into surrounding platforms.

codecov · 2023-08-23T12:08:51Z

Codecov Report

Patch coverage: 81.63% and project coverage change: +0.10% 🎉

Comparison is base (b4d3966) 66.26% compared to head (64299b3) 66.36%.
Report is 19 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5319      +/-   ##
=============================================
+ Coverage      66.26%   66.36%   +0.10%     
+ Complexity     15179    15178       -1     
=============================================
  Files           1789     1785       -4     
  Lines          69300    69180     -120     
  Branches        7355     7336      -19     
=============================================
- Hits           45923    45914       -9     
+ Misses         20890    20786     -104     
+ Partials        2487     2480       -7

Files Changed	Coverage Δ
...aph_builder/module/islandpruning/PruneIslands.java	`77.81% <0.00%> (ø)`
...tripplanner/inspector/raster/AreaEdgeRenderer.java	`14.28% <0.00%> (ø)`
.../opentripplanner/routing/linking/VertexLinker.java	`87.60% <60.00%> (-0.05%)`	⬇️
...pplanner/graph_builder/module/osm/OsmDatabase.java	`78.64% <66.66%> (-0.29%)`	⬇️
.../graph_builder/module/osm/WalkableAreaBuilder.java	`88.34% <90.00%> (+0.97%)`	⬆️
...tripplanner/openstreetmap/OpenStreetMapParser.java	`96.72% <100.00%> (ø)`
...tripplanner/openstreetmap/model/OSMMemberType.java	`100.00% <100.00%> (ø)`
...planner/openstreetmap/model/OSMRelationMember.java	`91.66% <100.00%> (+0.75%)`	⬆️
...pentripplanner/street/model/edge/AreaEdgeList.java	`95.45% <100.00%> (+4.54%)`	⬆️

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vesameskanen · 2023-08-23T14:08:58Z

There is a strange problem: graph build from the stopareas.pbf test file works as expected, and if I run the single test:

mvn surefire:test -Dtest=WalkableAreaBuilderTest

then the single test works fine. However, when running all unit tests, one of the tests fails. Also, rearranging assertions within WalkableAreaBuilderTest fixes the problem. I have no idea what is going on.

leonardehrenfried · 2023-08-23T15:26:57Z

I'm had a look if I can find some global state somewhere. There is the graph field which is shared between tests but Junit usually instances the test class for each test individually. Still it could be related to your problem, who knows.

vesameskanen · 2023-08-24T07:13:58Z

I still see random failures in WalkableAreaBuilderTest. Strange enough, another assertion sometimes fails locally as in github workflow. I don't think that the minor code change of this PR can explain the strange behaviour. Maybe an older random bug just started showing up.

leonardehrenfried · 2023-08-25T07:17:14Z

My theory is that the graph being reused is causing the test race conditions. Removing it has solved the problem for me at least for now. You can try this code: leonardehrenfried@1f71fa5

If it solves the problem, you can pull it into your branch.

leonardehrenfried · 2023-08-25T10:18:01Z

Hmm, that didn't solve it. :/

I'm currently looking around the code for sources of non-determinism and I'm testing one more thing. If that doesn't work, I give up and we can disable the test.

vesameskanen · 2023-08-25T10:26:16Z

OK, thanks. I found out that in test bench, vertex count is 18. When loaded to OTP (default settings + areaVisibility), vertex count is 35. I suspect that test setup somehow skips some graph processing actions. Anyway, it is strange that the failure and the actual assertion where fail happens changes randomly.

leonardehrenfried · 2023-08-25T10:31:45Z

So experimented a bit: https://github.com/leonardehrenfried/OpenTripPlanner/tree/flicker-test

I made the maxAreaNodes configurable per test and also made the visibilityVertices a private field that is properly encapsulated. In my tests it has run successfully three times on ci.

I still cannot really explain it but I think merging this branch is worth a try.

vesameskanen · 2023-08-25T11:04:51Z

Your new change set fixed random errors locally! Running the whole test bench or the single test class always succeeds.

I am still curious about the graph size difference (unit test graph vs normal OSM loaded into OTP) and will examine that part a bit more. Probably not relevant for approval of this PR.

vesameskanen · 2023-08-25T13:00:07Z

I managed to save the graph created during unit tests to disk. The picture shows the difference (test graph on the left, OSM loaded into OTP on the right): platform areas under testing are identical, but for some reason regular streets traveling under platforms are not included in the tested graph.

optionsome · 2023-08-25T18:17:05Z

  @Visibility(true)
-  public void test_calculate_vertices_area() {
+  @MaxAreaNodes(5)
+  public void test_calculate_vertices_area(TestInfo testInfo) {


Is using underscore in test method names ok in our conventions?

It's not. I don't know how that managed to get past review. I'm in favour of changing it.

optionsome · 2023-08-25T18:20:35Z

 import org.opentripplanner.street.model.vertex.VertexLabel.OsmNodeOnLevelLabel;
-import org.opentripplanner.transit.model.framework.Deduplicator;

+@Execution(ExecutionMode.SAME_THREAD)


Is this or other changes that were made in order to get tests to work before the final change still required for the tests to work?

Seems to work without it as well.

This line is not necessary but the rest of the changes is IMO worthwhile even if it doesn't fix the non-determinism, which I think was caused somehow by maxAreaNodes and the visibility vertices.

dekarl · 2023-08-26T18:26:45Z

I like the handling of isolated nodes (not connected to the road network), if they are part of a public transport relation of kind stop area.

It gives me a way forward with #5032 without making up physical ways. The alternative would be for the graph builder to look at every node that is not part of a way and check for public transport tags.

Now it just needs a mention in the documentation of this small deviation from the OSM tagging scheme. I think a short note will do https://docs.opentripplanner.org/en/v2.3.0/BoardingLocations/?h=boar#notes

e.g.

Nodes must be part of a way or a relation of kind stop_area to be considered for boarding location linking. Completely isolated nodes are currently ignored.

Example still needing a stop_area relation https://www.openstreetmap.org/node/2382791901#map=19/49.72095/8.84158&layers=D

Example that already has a relation https://www.openstreetmap.org/relation/13603101

leonardehrenfried · 2023-08-28T15:52:19Z

Actually, do you want to add Karl's suggestion to the BoardingLocation docs?

vesameskanen added 3 commits August 23, 2023 11:55

Update OSM stop area docs

8de36c4

Add test for stop linking in stop areas

549fc15

vesameskanen requested a review from a team as a code owner August 23, 2023 11:57

Test correct bus_stop node (not an entrance on same platform)

9def5c1

leonardehrenfried reviewed Aug 23, 2023

View reviewed changes

Comment thread src/main/java/org/opentripplanner/graph_builder/module/osm/OsmDatabase.java Outdated

vesameskanen added 2 commits August 24, 2023 09:29

Use positive OSM identifiers in stopareas.pbf test file

4e5bb50

Use very suitable existing method to check stop area linking nodes

2cbbd6e

leonardehrenfried requested a review from optionsome August 24, 2023 13:55

vesameskanen and others added 3 commits August 25, 2023 08:55

Update StopArea docs again

a68c23c

Refactor relation member type code

e52da54

Remove global state from test

1f71fa5

Make test deterministic

ea26554

Make visibilityVertices a private field

19523fb

leonardehrenfried added !Bug Apply to issues describing a bug and PRs witch fixes it. +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Aug 25, 2023

leonardehrenfried previously approved these changes Aug 25, 2023

View reviewed changes

optionsome reviewed Aug 25, 2023

View reviewed changes

Use consistent test naming and remove unnecessary annotation

2609224

vesameskanen dismissed leonardehrenfried’s stale review via 2609224 August 28, 2023 13:11

vesameskanen requested review from leonardehrenfried and optionsome August 28, 2023 13:53

leonardehrenfried previously approved these changes Aug 28, 2023

View reviewed changes

Document how OTP handles isolated OSM nodes

64299b3

vesameskanen dismissed leonardehrenfried’s stale review via 64299b3 August 29, 2023 07:32

leonardehrenfried approved these changes Aug 29, 2023

View reviewed changes

optionsome approved these changes Aug 29, 2023

View reviewed changes

vesameskanen merged commit 79035c4 into opentripplanner:dev-2.x Aug 29, 2023

vesameskanen deleted the extend-stoparea-linking branch August 29, 2023 07:59

t2gran pushed a commit that referenced this pull request Aug 29, 2023

Add changelog entry for #5319 [ci skip]

7f811fd

t2gran pushed a commit that referenced this pull request Aug 29, 2023

Bump serialization version id for #5319

2c1bc12

t2gran added this to the 2.4 milestone Sep 15, 2023

Uh oh!

Conversation

vesameskanen commented Aug 23, 2023

Summary

Unit tests

Documentation

Uh oh!

codecov Bot commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

vesameskanen commented Aug 23, 2023

Uh oh!

leonardehrenfried commented Aug 23, 2023

Uh oh!

vesameskanen commented Aug 24, 2023

Uh oh!

leonardehrenfried commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardehrenfried commented Aug 25, 2023

Uh oh!

vesameskanen commented Aug 25, 2023

Uh oh!

leonardehrenfried commented Aug 25, 2023

Uh oh!

vesameskanen commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vesameskanen commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

optionsome Aug 25, 2023 • 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!

leonardehrenfried Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

optionsome Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

vesameskanen Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

dekarl commented Aug 26, 2023

Uh oh!

leonardehrenfried commented Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Aug 23, 2023 •

edited

Loading

leonardehrenfried commented Aug 25, 2023 •

edited

Loading

vesameskanen commented Aug 25, 2023 •

edited

Loading

vesameskanen commented Aug 25, 2023 •

edited

Loading

optionsome Aug 25, 2023 •

edited

Loading