Bug fixes in stop area relation processing by vesameskanen · Pull Request #5166 · opentripplanner/OpenTripPlanner

vesameskanen · 2023-06-05T13:08:33Z

Summary

OpenTripPlannert has some special code which processes stop area relations. The purpose is to improve linking of platforms with the street network.

Previously, this code included several bugs and limitations:

All member nodes of the relation were linked against an area geometry, even if the nodes represented entrances on different level or layer than the target geometry. This generated strange shortcuts for example from subway platforms directly to the entrance above ground level.
Linking now handles all areas of relation, not only the first one
Linking now connects isolated entrance nodes with surrounding area edges. This helps to preserve the area in case it is not explicitly connected with the street network, and thus improves walk routing.

Images below show a subway station, where walk routing was badly broken. Second image shows the correct walk route.

Issue

5158

closes #5158

Unit tests

Three unit tests added. They ensure that:

Linking does not happen across different layers/levels, and not from the outside of an area
Linking handles multiple separate areas
Entrance points get explicit connection with the area edge, so that areas won't get pruned as islands

Documentation

New comments added to illuminate somewhat complicated logic
New page in docs folder

…odes E.g. transit entrances above ground should not get directly linked to subway platforms

…different layers/levels

Entrances and other unconnected nodes included in stop area relation were linked only with the first platform area. Now they are matched with all platforms. Walkable area builder won't create connections if node is outside a platform.

codecov · 2023-06-05T13:25:24Z

Codecov Report

Patch coverage: 86.92% and project coverage change: +0.39 🎉

Comparison is base (255e327) 65.05% compared to head (d053da2) 65.45%.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5166      +/-   ##
=============================================
+ Coverage      65.05%   65.45%   +0.39%     
- Complexity     14332    14494     +162     
=============================================
  Files           1748     1754       +6     
  Lines          67875    68021     +146     
  Branches        7234     7238       +4     
=============================================
+ Hits           44158    44522     +364     
+ Misses         21272    21031     -241     
- Partials        2445     2468      +23

Impacted Files	Coverage Δ
...er/ext/transmodelapi/model/TransportModeSlack.java	`88.40% <ø> (+4.84%)`	⬆️
...entripplanner/openstreetmap/model/OSMWithTags.java	`88.88% <66.66%> (-1.68%)`	⬇️
.../preferences/ItineraryFilterPreferencesMapper.java	`75.00% <75.00%> (ø)`
...api/mapping/preferences/WalkPreferencesMapper.java	`75.00% <75.00%> (ø)`
.../mapping/preferences/TransitPreferencesMapper.java	`78.94% <78.94%> (ø)`
...i/mapping/preferences/StreetPreferencesMapper.java	`87.50% <87.50%> (ø)`
...mapping/preferences/TransferPreferencesMapper.java	`87.50% <87.50%> (ø)`
...i/model/framework/StreetModeDurationInputType.java	`70.37% <89.47%> (+11.39%)`	⬆️
...api/mapping/preferences/BikePreferencesMapper.java	`91.66% <91.66%> (ø)`
.../graph_builder/module/osm/WalkableAreaBuilder.java	`89.72% <91.66%> (+3.89%)`	⬆️
... and 4 more

... and 127 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

leonardehrenfried · 2023-06-05T13:34:14Z

    }
  }

+  private int getLevel(OSMWithTags o) {


This should be part of OSMWithTags.

Also, please remove the duplication in the method.

leonardehrenfried · 2023-06-05T13:39:09Z

-        } else {
-          issueStore.add(new TooManyAreasInRelation(relation));
-        }
+        platformAreas.add(areaWaysById.get(member.getRef()));


I think we should make platformAreas a SetMultimap: https://guava.dev/releases/19.0/api/docs/com/google/common/collect/SetMultimap.html

That way we can also make sure that we don't return null.

leonardehrenfried

Please consider my suggestions.

Stop area which does not have entrances or other link points is really not any kind of error

vesameskanen · 2023-06-06T07:25:23Z

Thanks for good observations, I believe I got them all addressed.

leonardehrenfried · 2023-06-07T20:46:35Z

+- Connect entrance points to the street network
+- Platform and its entrance point must have the same `level` tag value. Also matching by default value zero is accepted.
+- If `level` tag is not set, `layer` tag is also considered
+


Can you link to an example of such a relation? I would find it quite useful.

leonardehrenfried

I have two further requests.

leonardehrenfried · 2023-06-08T09:55:11Z

* I am wondering should we actually remove the 'link all nodes inside and at the same  level' behaviour. Linking could handle only entrance or elevator tagged nodes , or endpoints of stair cases leading to a platform. Now all kind of stop_position etc. nodes which are not relevant for walk routing may get linked.

I didn't know this feature existed either :). So for years I have been advising people to link the end of the stairs to the platform edge and since you've found it challenging to find an example of it being done correctly I'm in favour of this.

Linking stop_positions seems very wrong indeed.

vesameskanen · 2023-06-08T10:47:53Z

I will continue later with a second PR. Level handling looks complicated as well, and it may take some time to sort it out.

- Test that elevators get linked - Test that stop positions will not get linked

vesameskanen · 2023-06-20T12:18:29Z

Here's the graph from the platform test file.

leonardehrenfried · 2023-06-20T12:23:48Z

+    var levels = getMultiTagValues(levelTags);
+    if (levels.isEmpty()) {
+      // default
+      levels.add("0");


Suggested change

levels.add("0");

Set.of("0");

leonardehrenfried

You can use the static Set constructor.

…ags.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

optionsome

Do we have to care about elevator ways in this case which may have level information or are we just concerned about nodes? https://www.openstreetmap.org/way/465149338

leonardehrenfried · 2023-06-20T13:09:48Z

I don't know if it is implemented like that already but I would expect it to work if it is correctly linked to the rest of the OSM network.

leonardehrenfried · 2023-06-20T13:10:23Z

So there the level tag is purely informative since we don't have to do magic linking of the nodes.

…ags.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

* Add changelog entry for opentripplanner#5100 [ci skip] * refactor: Add proper progress tracking for GraphQL timeouts. * refactor: Fix spelling 'guarantied' -> 'guaranteed' * review: Fix documentation * feature: Trace HTTP request headers * Add changelog entry for opentripplanner#5081 [ci skip] * Add changelog entry for opentripplanner#5091 [ci skip] * Add changelog entry for opentripplanner#5133 [ci skip] * Remove unused code * Generate full POM for shaded jar [ci skip] * Add elevation data to Transmodel API * Finetune elevation format * Update documentation * Add changelog entry for opentripplanner#5142 [ci skip] * fix: Make sure the log key is removed from the Grizzly thread log context * Apply suggestions from code review Co-authored-by: Thomas Gran <t2gran@gmail.com> * Remove San Francisco fare calculator * Remove TimeBasedVehicleRentalFareService * Add documentation about deleted calculators * Remove empty lines * Refactor null check on from and to vertices * review: Apply review feedback * Apply suggestions from code review Co-authored-by: Leonard Ehrenfried <mail@leonard.io> * feature: Add sanity check for HTTP header value The value must match `[^\p{Cntrl}\v]{1,512}` * test: Add test regular expression for HTTP header value * Apply suggestions from code review Co-authored-by: Leonard Ehrenfried <mail@leonard.io> * feature: Remove batch query from Transmodel API * refactor: re-generate doc * fix(deps): update dependency com.google.guava:guava to v32 * fix(deps): update dependency com.graphql-java:graphql-java to v20.3 * Add changelog entry for opentripplanner#5130 [ci skip] * fix(deps): update dependency com.google.guava:guava to v32 * Use git to figure out last-modified date * Debug last modified check * Increase fetch depth * Make update frequency a Duration * fix(deps): update dependency com.fasterxml.jackson.core:jackson-annotations to v2.15.2 * Update log messages * Update docs * Remove link to 1.x-dev docs * refactor: Avoid creating builders unnecessary. * test: Add test for bug in TripRequestMapperTest * Add changelog entry for opentripplanner#5131 [ci skip] * Bump serialization version id for opentripplanner#5131 * Add changelog entry for opentripplanner#5141 [ci skip] * Bump serialization version id for opentripplanner#5141 * Improve money documentation * Implement stop sequence in GraphQL * Add stop sequence and test * Fix docs, formatting and tests * Add test for walk step mapping * fix: Fix bug in maxDirectDurationForMode and refactor - The code is not DRY, so I refactored it - this also fixes the bug * refactor: Extract mappers out off PreferencesMapper * fix(deps): update dependency com.google.cloud:libraries-bom to v26.15.0 * fix(deps): update dependency org.onebusaway:onebusaway-gtfs to v1.4.3 * Make absolute direction optional * Add test for GraphQL API * Add changelog entry for opentripplanner#5140 [ci skip] * Update documentation * Add changelog entry for opentripplanner#5145 [ci skip] * Consider level and layer tags when linking public transit stop area nodes E.g. transit entrances above ground should not get directly linked to subway platforms * Add test for ensuring that entrances do not link to platforms across different layers/levels * Update documentation and mapping * test: Add regression test. * fix: Fix validation of flex area, assert isComplete and isConsistent, before isStopTimesIncreasing * refactor: Cleanup AbstractStopTimeAdaptor * Improve documentation * Handle stop areas with many platforms properly Entrances and other unconnected nodes included in stop area relation were linked only with the first platform area. Now they are matched with all platforms. Walkable area builder won't create connections if node is outside a platform. * Update documenation * Create interline transfers for trips that share the same service date and block * Connect area boundary to entrance points inside it to prevent pruning * Add better test data for testing area processing of stop_area relations * Validate to/from in routing request * Add changelog entry for opentripplanner#5152 [ci skip] * Bump serialization version id for opentripplanner#5152 * Stop area linking tests * Changing default value for earlyStartSec * Add some documetation about stop area relations * Fix formatting * Add support for mapping NeTEx operating day in operating period * Add error mapping in REST API * Update documentation * Add new doc page to mkdocs.yml * Move getLevel to OSMWithTags andd simplify it * Test also layer tag relevance in stop area processing * Fix misleading data report issue content from stop area processing * Use multimap for storing stop area link nodes * Remove obsolete issue Stop area which does not have entrances or other link points is really not any kind of error * Add changelog entry for opentripplanner#5147 [ci skip] * Improve updater log messages * refactor: Apply code review - Change `earlyStartSec:int` to `earlyStart:Duration` - Add more doc on parameter * Relax validity check for flex trip with null duration * Make FlexPath fields final * refactor: Make `SiriSXUpdaterParameters#timeout` a Duration * Apply suggestions from code review * refactor: Make Siri Updaters use Duration, not int, for reminding parameters - This also remove a bit of unnecessary mapping code. * Update src/main/java/org/opentripplanner/standalone/config/routerconfig/updaters/SiriSXUpdaterConfig.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io> * chore(deps): update dependency org.apache.maven.plugins:maven-surefire-plugin to v3.1.2 * Refactor shutdown hook * Add changelog entry for opentripplanner#5159 [ci skip] * Update pull_request_template.md [ci skip] * fix(deps): update dependency com.graphql-java:graphql-java to v20.4 * Use hashmultimap in src/main/java/org/opentripplanner/graph_builder/module/osm/OsmDatabase.java> Co-authored-by: Leonard Ehrenfried <mail@leonard.io> * Add HashMultimap import * Refactor first/last date getters * Split method in two * Update src/main/java/org/opentripplanner/graph_builder/module/interlining/InterlineProcessor.java Co-authored-by: Thomas Gran <t2gran@gmail.com> * More accurate tagging instructions, one example relation linked * Log warning if GBFS status reports unexpected vehicle type * Code cleanup * refactor: Cleanup TransitRouter and AccessEgressMapper * refactor: Sort values in DurationForEnum.toString to make it deterministic * refactor: Add State.containsModeWalkOnly() and DefaultAccessEgress.isWalkOnly() These methods will make it simpler to filter access/egress later * refactor: Add Duration#requireNonNegative(Duration) : Duration to DurationUtils * refactor: Implement openingHoursToString() for AccessEgress for testing Having to brows through many classes and hairy logic is time-consuming when debugging FLEX access/egress, this simplifies the process. * refactor: Move RaptorConstants into raptor.api.model package * feature: Search before the earliest-departure-time in Raptor with searchWindowAccessSlack. * refactor: Move AccessEgresses to street package * refactor: Cleanup DoubleUtils * refactor: Add requireXyz to IntUtils * refactor: Add Cost value-object * refactor: Improve int and double utilities * refactor: Small cleanups * Apply suggestions from code review Co-authored-by: Thomas Gran <t2gran@gmail.com> * Formatting * Add changelog entry for opentripplanner#5161 [ci skip] * Add changelog entry for opentripplanner#5162 [ci skip] * Add changelog entry for opentripplanner#5168 [ci skip] * Apply review suggestions * Apply review suggestions * Fix bicyle optimise type in TransmodelApi * Add test for optimize type in GraphQL API * fix(deps): update dependency com.google.guava:guava to v32.0.1-jre * Add changelog entry for opentripplanner#5167 [ci skip] * Add reusable method * Add union type for stop position * Simplify type resolving * Use interface type in data fetcher * Applied review suggestion * Add documentation * Add documentation * Apply review feedback Co-authored-by: Joel Lappalainen <lappalj8@gmail.com> * Generate new SiriUpdater doc * Add changelog entry for opentripplanner#5169 [ci skip] * Add changelog entry for opentripplanner#5175 [ci skip] * Add changelog entry for opentripplanner#5164 [ci skip] * Add changelog entry for opentripplanner#5165 [ci skip] * review: Remove serialVersionUID * Validate that from and to temporary vertices are distinct * review: Extract TestVehicleRentalStationBuilder * Reduce log severity for non-optimized transfers * Return an int as the stopPosition for StopTimes * doc: Move JavaDoc to accessors, fix typo. * fix: Improve error handling and prevent OTP from going down when connecting to external http services. The VehicleRentalServiceDirectoryFetcher went down with a IllegalSateException when the http endpoint failed. Instead of returning null in some error-cases and throwing IOExceptions in others the HttpUtils is changed to throw an IOException in all cases. This make it more robust, and the checked exception forces the client to handle it. * Use Finland OSM mapping as basis for constant speed mapper * Add support for taxi mode * Rename ConstansSpeedMapper to describe the new super class * Add changelog entry for opentripplanner#5153 [ci skip] * Update micrometer.version to v1.11.1 * Update src/main/java/org/opentripplanner/routing/algorithm/mapping/RaptorPathToItineraryMapper.java Co-authored-by: Thomas Gran <t2gran@gmail.com> * Apply review feedback * Update dependency ch.qos.logback:logback-classic to v1.4.8 * Add changelog entry for opentripplanner#5135 [ci skip] * Separate words with underscore in osm tag mapper enum value * Update docs * Bump serialization version id for opentripplanner#5176 * Make stop area linking more precise and capable to handle elevators * Add changelog entry for opentripplanner#5181 [ci skip] * Use EnumSet instead of Stream * Add changelog entry for opentripplanner#5183 [ci skip] * Add changelog entry for opentripplanner#5179 [ci skip] * Fix formatting in RouterConfig.md * improve: add language argument to Quay and StopPlace types deprecate lang arguments * fix: the overloaded getLocale method does not use the language argument * improve: extract shared code into helper method in transmodel GqlUtil * add test for GqlUtil.getLocale * Fix default value for bicycle safety report * Update dependency org.apache.maven.plugins:maven-shade-plugin to v3.5.0 * Update dependency net.logstash.logback:logstash-logback-encoder to v7.4 * Update dependency org.mockito:mockito-core to v5.4.0 * Add more tests for stop area linking - Test that elevators get linked - Test that stop positions will not get linked * Fix typo * Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io> * Return set * Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com> * Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com> * Add changelog entry for opentripplanner#5166 [ci skip] --------- Co-authored-by: Leonard Ehrenfried <mail@leonard.io> Co-authored-by: OTP Changelog Bot <changelog-bot@opentripplanner.org> Co-authored-by: Thomas Gran <t2gran@gmail.com> Co-authored-by: vpaturet <46598384+vpaturet@users.noreply.github.com> Co-authored-by: Vincent Paturet <vincent.paturet@entur.org> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: OTP Serialization Version Bot <serialization-version-bot@opentripplanner.org> Co-authored-by: Vesa Meskanen <vesa.meskanen@cgi.com> Co-authored-by: Joel Lappalainen <lappalj8@gmail.com> Co-authored-by: Lasse Tyrihjell <lassetyr@gmail.com> Co-authored-by: Vesa Meskanen <vesa@realsoft.com> Co-authored-by: Tom Erik Støwer <testower@gmail.com>

vesameskanen added 7 commits June 2, 2023 07:09

Consider level and layer tags when linking public transit stop area n…

b435915

…odes E.g. transit entrances above ground should not get directly linked to subway platforms

Add test for ensuring that entrances do not link to platforms across …

040b0c9

…different layers/levels

Handle stop areas with many platforms properly

ce3b7f9

Entrances and other unconnected nodes included in stop area relation were linked only with the first platform area. Now they are matched with all platforms. Walkable area builder won't create connections if node is outside a platform.

Connect area boundary to entrance points inside it to prevent pruning

2052068

Add better test data for testing area processing of stop_area relations

93f2ec6

Stop area linking tests

b057f0a

Add some documetation about stop area relations

46553e9

vesameskanen requested a review from a team as a code owner June 5, 2023 13:08

Fix formatting

c675ac5

vesameskanen added !Bug Apply to issues describing a bug and PRs witch fixes it. !Improvement A functional improvement or micro feature labels Jun 5, 2023

leonardehrenfried self-requested a review June 5, 2023 13:30

leonardehrenfried reviewed Jun 5, 2023

View reviewed changes

Comment thread docs/StopAreas.md

leonardehrenfried reviewed Jun 5, 2023

View reviewed changes

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

leonardehrenfried requested changes Jun 5, 2023

View reviewed changes

vesameskanen added 6 commits June 6, 2023 07:05

Add new doc page to mkdocs.yml

ace0105

Move getLevel to OSMWithTags andd simplify it

226ebaa

Test also layer tag relevance in stop area processing

a522754

Fix misleading data report issue content from stop area processing

61e7df5

Use multimap for storing stop area link nodes

458d646

Remove obsolete issue

4f62827

Stop area which does not have entrances or other link points is really not any kind of error

vesameskanen requested a review from leonardehrenfried June 6, 2023 07:25

t2gran added this to the 2.4 (next release) milestone Jun 6, 2023

leonardehrenfried reviewed Jun 7, 2023

View reviewed changes

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

leonardehrenfried requested changes Jun 7, 2023

View reviewed changes

leonardehrenfried previously approved these changes Jun 8, 2023

View reviewed changes

leonardehrenfried requested a review from optionsome June 8, 2023 14:29

vesameskanen added 2 commits June 15, 2023 09:51

Make stop area linking more precise and capable to handle elevators

0861247

Add more tests for stop area linking

bd7cf4f

- Test that elevators get linked - Test that stop positions will not get linked

vesameskanen dismissed leonardehrenfried’s stale review via bd7cf4f June 20, 2023 12:16

Fix typo

5e76572

vesameskanen requested a review from leonardehrenfried June 20, 2023 12:22

leonardehrenfried reviewed Jun 20, 2023

View reviewed changes

Comment thread src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java Outdated

leonardehrenfried reviewed Jun 20, 2023

View reviewed changes

leonardehrenfried requested changes Jun 20, 2023

View reviewed changes

vesameskanen and others added 2 commits June 20, 2023 15:35

Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithT…

424a79f

…ags.java Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

Return set

c761ebc

vesameskanen requested a review from leonardehrenfried June 20, 2023 12:51

leonardehrenfried previously approved these changes Jun 20, 2023

View reviewed changes

optionsome reviewed Jun 20, 2023

View reviewed changes

Comment thread src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java Outdated

Comment thread src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java Outdated

Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithT…

93581c2

…ags.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

vesameskanen dismissed leonardehrenfried’s stale review via 93581c2 June 20, 2023 13:22

Update src/main/java/org/opentripplanner/openstreetmap/model/OSMWithT…

d053da2

…ags.java Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

optionsome approved these changes Jun 20, 2023

View reviewed changes

leonardehrenfried approved these changes Jun 20, 2023

View reviewed changes

vesameskanen merged commit 01218bc into opentripplanner:dev-2.x Jun 20, 2023

vesameskanen deleted the stoparealinking branch June 20, 2023 13:44

t2gran pushed a commit that referenced this pull request Jun 20, 2023

Add changelog entry for #5166 [ci skip]

1384a11

vesameskanen mentioned this pull request Aug 23, 2023

Extend stop area relation linking to include bus stop and platform nodes #5319

Merged

Uh oh!

Conversation

vesameskanen commented Jun 5, 2023 • edited by leonardehrenfried Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

leonardehrenfried Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

vesameskanen commented Jun 6, 2023

Uh oh!

leonardehrenfried Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

vesameskanen commented Jun 8, 2023

Uh oh!

vesameskanen commented Jun 20, 2023

Uh oh!

Uh oh!

leonardehrenfried Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

vesameskanen Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leonardehrenfried commented Jun 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vesameskanen commented Jun 5, 2023 •

edited by leonardehrenfried

Loading

codecov Bot commented Jun 5, 2023 •

edited

Loading

leonardehrenfried commented Jun 8, 2023 •

edited

Loading

leonardehrenfried commented Jun 20, 2023 •

edited

Loading