Adaptive street graph island pruning #4688
Conversation
It is very useful to see the complexity of the removed island
- Size thresholds depend on distance - Consider ferry transport mode
With default parameters, aggressive adaptive pruning would erase relevant parts from the small test data set
…blems, not only access restrictions
Default settings may prune too small OSM test data sets.
Codecov ReportBase: 61.64% // Head: 61.63% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #4688 +/- ##
=============================================
- Coverage 61.64% 61.63% -0.01%
- Complexity 12637 12665 +28
=============================================
Files 1611 1612 +1
Lines 64360 64539 +179
Branches 6978 7002 +24
=============================================
+ Hits 39677 39781 +104
- Misses 22447 22520 +73
- Partials 2236 2238 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
As soon as there are more than 10 issues of a kind, sorting becomes an invaluable feature. In graph pruning, number of issues is counted in thousands.
src/main/java/org/opentripplanner/graph_builder/module/PruneIslands.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opentripplanner/graph_builder/module/Subgraph.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I really like the thorough documentation page. Could you add it to mkdocs.yml so that it shows up in the menu?
Also, since we now have 4 parameters to control the pruning, should we have our own config object for it?
// build-config.json
{
"islandPruning": {
"adaptivePruningDistance": 250,
... ...
}
}
|
Also, could you add example values in https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/test/resources/standalone/config/build-config.json ? That way they will end up in the documentation as well. |
- Create only one issue per sub graph about unlinked stops. Rename the issue to reflect the change. - Include relevant details about pruning changes and traverse mode so that issue can be understood
Co-authored-by: Leonard Ehrenfried <[email protected]>
Co-authored-by: Leonard Ehrenfried <[email protected]>
Co-authored-by: Leonard Ehrenfried <[email protected]>
leonardehrenfried
added
the
+Config Change
|
Can you click "re-request review" when you have found the logic bug? |
There was a problem hiding this comment.
I saw some lines in the GraphIsland reports that were like Pruned WALK subgraph area splitter at (24.62297726728413, 60.16452804217827, NaN) containing vertex '{area splitter at (24.62297726728413, 60.16452804217827, NaN) lat,lng=60.16452804217827,24.62297726728413}' at (24.622977, 60.164528) of 288 street vertices and 0 stops. Edge changes: 809 to nothru, 0 to no traversal, 0 erased. What is that "third" NaN coordinate value?
src/main/java/org/opentripplanner/graph_builder/issue/api/DataImportIssue.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * search radius as meters when looking for island neighbours | ||
| */ | ||
| private double adaptivePruningDistance; |
There was a problem hiding this comment.
Does this really need to be double? Couldn't it just be int?
There was a problem hiding this comment.
Another thing, should we defined the unit in the parameter name or not?
There was a problem hiding this comment.
Right, I considered both these questions, and was not sure what to choose. Distance is natively a decimal value, although one meter accuracy is surely enough.
I'm not sure if adding unit is good or bad, e.g. 'adaptivePruningDistanceMeters'. Is there a standard convention or a decision about such one already?
There was a problem hiding this comment.
We discussed this in the dev meeting and came to the conclusion that this should be int, not double. Name can stay as is. The javadoc should be on a getter method, not on the field.
src/main/java/org/opentripplanner/graph_builder/module/Subgraph.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opentripplanner/standalone/config/buildconfig/IslandPruningConfig.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Joel Lappalainen <[email protected]>
…g/IslandPruningConfig.java Co-authored-by: Joel Lappalainen <[email protected]>
…pPlanner into adaptive-pruning
|
We discussed this pr today in the developer meeting and @leonardehrenfried pointed out that it would make sense to convert the issue classes (at least the ones heavily edited here) as records so less code is needed and they fit the use case pretty well. |
Also, do not let islands overlap by nothru edges as this can generate islands, which are almost identical (only a few unique normal edges, lots of shared nothru edges). Almost identical islands make issue report harder to use.
Instance methods are now in use, so there is no need to pass constant data around as long complicated function parameter lists.
After pruning changes, one footway=highway island no longer got car nothru restriction. This appears acceptable as the mentioned way does not have CAR traverse mode at all. Car nothru makes no sense for such a way.
- Code now tracks if processed edges actually change. Logged statistics reflect more accurately street graph changes. - Example vertex for graph island issue is taken from an edge which changed in pruning, giving a more useful link to OSM data.
|
Conversion of issue classes to simpler records will be done in another PR. |
|
Can you resolve the conflicts? |
|
Conflicts resolved. |
| public static final String FMT = "Area %s is too complicated (%s > %s )"; | ||
| public static final String FMT = "Area %s is too complicated (%s > %s)"; | ||
| public static final String HTMLFMT = | ||
| "Area <a href='http://www.openstreetmap.org/%s'>'%s'</a> is too complicated (%s > %s)"; |
There was a problem hiding this comment.
Each OSM element has a method to generate an OSM link:
However, you can change that in your follow-up PR which also converts the records.
| ) | ||
| ); | ||
| OSMWithTags osm = group.getSomeOSMObject(); | ||
| String id = (osm instanceof OSMWay) ? "way/" + osm.getId() : "relation/" + osm.getId(); |
There was a problem hiding this comment.
In your follow up you can also replace his with getOpenstreetmapLink.
|
The documentation is now on https://docs.opentripplanner.org/en/dev-2.x/IslandPruning/ However, I think it didn't come out how you wanted it as the newlines look pretty broken. |
|
True, image captions show up different from github's presentation of the .md file. I will fix them |
4 participants
Summary
Street graph pruning uses distance and transport mode based heuristics to decide which sub graphs should be pruned.
Pruning should remove quite large disconnected sub graphs because they cause great damage to routing (see the new document page included in the PR). However, small real geographic islands which may have ferry connections should be always retained.
New processing steps may slow down graph building a bit. Helsinki region graph build changed from 150 seconds to 200 seconds. Distance heuristics can be turned off using build-config settings.
Unit tests
Adaptive pruning is tested with an OSM snippet which contains a small network of 3 separated graphs.
Documentation
New document page with examples added to docs folder