Skip to content

Configure the import of OSM extracts individually#4419

Merged
vpaturet merged 8 commits intoopentripplanner:dev-2.xfrom
entur:otp2_override_osm_configuration
Sep 2, 2022
Merged

Configure the import of OSM extracts individually#4419
vpaturet merged 8 commits intoopentripplanner:dev-2.xfrom
entur:otp2_override_osm_configuration

Conversation

@vpaturet
Copy link
Copy Markdown
Contributor

@vpaturet vpaturet commented Aug 29, 2022

Summary

Update the OpenStreetMap graph build module to make use of feed-specific parameters for timezone and country-specific rules (WayPropertySet)

Issue

Implement issue #4418
Requires PR #4399

Unit tests

Documentation

Updated graph builder configuration as requested in #4399

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 29, 2022

Codecov Report

Merging #4419 (2cf040c) into dev-2.x (00e710e) will decrease coverage by 0.00%.
The diff coverage is 77.14%.

@@              Coverage Diff              @@
##             dev-2.x    #4419      +/-   ##
=============================================
- Coverage      58.22%   58.21%   -0.01%     
- Complexity     11204    11205       +1     
=============================================
  Files           1480     1480              
  Lines          59131    59165      +34     
  Branches        6783     6783              
=============================================
+ Hits           34431    34445      +14     
- Misses         22644    22661      +17     
- Partials        2056     2059       +3     
Impacted Files Coverage Δ
...anner/standalone/config/feed/OsmExtractConfig.java 100.00% <ø> (+12.50%) ⬆️
...pplanner/graph_builder/module/osm/OSMDatabase.java 77.09% <50.00%> (-0.28%) ⬇️
...ipplanner/openstreetmap/OpenStreetMapProvider.java 77.77% <64.70%> (-5.56%) ⬇️
.../graph_builder/module/osm/WalkableAreaBuilder.java 80.40% <66.66%> (-0.43%) ⬇️
.../graph_builder/module/osm/OpenStreetMapModule.java 84.17% <80.76%> (-0.48%) ⬇️
...ipplanner/openstreetmap/OSMOpeningHoursParser.java 89.10% <100.00%> (+0.16%) ⬆️
...tripplanner/openstreetmap/OpenStreetMapParser.java 96.77% <100.00%> (+0.13%) ⬆️
...entripplanner/openstreetmap/model/OSMWithTags.java 87.80% <100.00%> (-3.03%) ⬇️
...entripplanner/openstreetmap/model/OSMRelation.java 71.42% <0.00%> (-14.29%) ⬇️
...g/opentripplanner/openstreetmap/model/OSMNode.java 88.88% <0.00%> (-5.56%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vpaturet
Copy link
Copy Markdown
Contributor Author

vpaturet commented Aug 29, 2022

There are 2 street graph properties that are based on WayPropertySets and that are applied at graph-level:

Driving direction does not seem to be used. Should it be removed?
Intersection traversal cost model is used in StreetEdge:


Does it make sense to refactor it so that it becomes feed-specific?

@hannesj
Copy link
Copy Markdown
Contributor

hannesj commented Aug 29, 2022

DrivingDirection is used in SimpleIntersectionTraversalCostModel, so it needs to be kept. For now, we could remove it from the WayPropertySets, and configure it directly on the graph level? Same with IntersectionTraversalCostModel

@vpaturet
Copy link
Copy Markdown
Contributor Author

DrivingDirection is used in SimpleIntersectionTraversalCostModel, so it needs to be kept. For now, we could remove it from the WayPropertySets, and configure it directly on the graph level? Same with IntersectionTraversalCostModel

Ok, then the implicit assumption is that driving direction and intersection traversal cost model is uniform across all covered countries. This can be implemented as such and revisited later on.

@hannesj
Copy link
Copy Markdown
Contributor

hannesj commented Aug 29, 2022

That sounds good.

@hannesj
Copy link
Copy Markdown
Contributor

hannesj commented Aug 30, 2022

Rename wayPropertySet as osmTagMapping in the config, internal classes can be renamed in the future

@hannesj
Copy link
Copy Markdown
Contributor

hannesj commented Sep 1, 2022

Take into account config changes in #4399

@leonardehrenfried leonardehrenfried added the +Config Change This PR might require the configuration to be updated. label Sep 1, 2022
…figuration

# Conflicts:
#	src/main/java/org/opentripplanner/openstreetmap/OpenStreetMapProvider.java
@vpaturet vpaturet added +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR !New Feature A functional feature targeting the end user. labels Sep 2, 2022
@vpaturet vpaturet marked this pull request as ready for review September 2, 2022 07:21
@vpaturet vpaturet requested a review from a team as a code owner September 2, 2022 07:21

The osm section of `build-config.json` allows you to override the default behavior of scanning
for OpenStreetMap files in the [base directory](Configuration.md#Base Directory). You can specify data
for OpenStreetMap files in the [base directory](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/docs/Configuration.md#Base-Directory). You can specify data
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these types of links work nicely with read the docs on http://docs.opentripplanner.org/en/latest/ ?

@vpaturet vpaturet merged commit 98b6513 into opentripplanner:dev-2.x Sep 2, 2022
t2gran pushed a commit that referenced this pull request Sep 2, 2022
t2gran pushed a commit that referenced this pull request Sep 2, 2022
@t2gran t2gran added this to the 2.2 milestone Oct 25, 2022
@t2gran t2gran deleted the otp2_override_osm_configuration branch March 31, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR +Config Change This PR might require the configuration to be updated. !New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants