conveyal / r5

Developed to power Conveyal's web-based interface for scenario planning and land-use/transport accessibility analysis, R5 is our routing engine for multimodal (transit/bike/walk/car) networks with a particular focus on public transit
https://conveyal.com/learn
MIT License
287 stars 74 forks source link

R5 fails to load osm clipped using OSMOSIS #276

Open zishanbilal opened 7 years ago

zishanbilal commented 7 years ago

R5 fails to load clipped osm using OSMOSIS to clip a sub-region from a larger region. Where as when with i run it with OpenTripPlanner it work perfectly fine and generates Graph.obj. I run through the code to debug the scenario, below are my findings.

While loading osm StreetLayer.loadFromOsm(osm, true, false) when it start making edges for the way that has no intersection node osm.intersectionNodes.contains(way.nodes[n]) and it still go to make edge StreetLayer.makeEdge(way, beginIdx, n, entry.getKey()) with beginIdx = 0 and n = last-index-of-nodes, it fails to find Node from osm.nodes.get(osmNodeId), inside StreetLayer.getVertexIndexForOsmNode(long osmNodeId) for begainOsmNodeIdand throws null pointer exception as follows:

SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/C:/Users/Zeeshan%20Bilal/.m2/repository/org/slf4j/slf4j-simple/1.7.10/slf4j-simple-1.7.10.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/C:/Users/Zeeshan%20Bilal/.m2/repository/ch/qos/logback/logback-classic/1.1.3/logback-classic-1.1.3.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.SimpleLoggerFactory]
[main] INFO com.conveyal.r5.point_to_point.PointToPointRouterServer - Arguments: [--build, c:/northstar/data/r5/berkeley]
[main] INFO com.conveyal.r5.transit.TransportNetwork - File 'c:\northstar\data\r5\berkeley\build-config.json' is not present. Using default configuration.
[main] INFO com.conveyal.r5.transit.TransportNetwork - Found GTFS file c:\northstar\data\r5\berkeley\ac-transit.gtfs.zip
[main] INFO com.conveyal.r5.transit.TransportNetwork - Found GTFS file c:\northstar\data\r5\berkeley\bart.gtfs.zip
[main] INFO com.conveyal.r5.transit.TransportNetwork - Found OSM file c:\northstar\data\r5\berkeley\berkeley-withservice.osm.pbf
[main] WARN com.conveyal.r5.transit.TransportNetwork - Skipping non-input file 'c:\northstar\data\r5\berkeley\Graph.obj'
[main] WARN com.conveyal.r5.transit.TransportNetwork - Skipping non-input file 'c:\northstar\data\r5\berkeley\osm.mapdb'
[main] WARN com.conveyal.r5.transit.TransportNetwork - Skipping non-input file 'c:\northstar\data\r5\berkeley\osm.mapdb.p'
Summarizing builder config: build-config.json
{
  "htmlAnnotations" : false,
  "maxHtmlAnnotationsPerFile" : 1000,
  "transit" : true,
  "useTransfersTxt" : false,
  "parentStopLinking" : false,
  "stationTransfers" : false,
  "subwayAccessTime" : 2.0,
  "streets" : true,
****  "embedRouterConfig" : true,
  "areaVisibility" : false,
  "matchBusRoutesToStreets" : false,
  "fetchElevationUS" : false,
  "speeds" : {
    "units" : "mph",
    "values" : {
      "secondary" : 35,
      "tertiary_link" : 25,
      "primary_link" : 25,
      "tertiary" : 25,
      "living_street" : 5,
      "motorway_link" : 35,
      "trunk" : 55,
      "secondary_link" : 25,
      "motorway" : 65,
      "trunk_link" : 35,
      "primary" : 45
    },
    "defaultSpeed" : 25
  },
  "staticBikeRental" : false,
  "staticParkAndRide" : true,
  "staticBikeParkAndRide" : false,
  "bikeRentalFile" : null,
  "analysisFareCalculator" : null
}
[main] INFO com.conveyal.osmlib.OSM - Reading OSM DB from: c:\northstar\data\r5\berkeley\osm.mapdb
[main] INFO com.conveyal.osmlib.OSM - Not reading from file since database is already filled!
[main] INFO com.conveyal.osmlib.OSM - Detecting intersections...
[main] INFO com.conveyal.osmlib.OSM - Done detecting intersections.
[main] INFO com.conveyal.r5.streets.StreetLayer - Making street edges from OSM ways...
Exception in thread "main" java.lang.NullPointerException
    at com.conveyal.r5.streets.StreetLayer.getVertexIndexForOsmNode(StreetLayer.java:916)
    at com.conveyal.r5.streets.StreetLayer.makeEdge(StreetLayer.java:959)
    at com.conveyal.r5.streets.StreetLayer.loadFromOsm(StreetLayer.java:264)
    at com.conveyal.r5.streets.StreetLayer.loadFromOsm(StreetLayer.java:170)
    at com.conveyal.r5.transit.TransportNetwork.fromFiles(TransportNetwork.java:166)
    at com.conveyal.r5.transit.TransportNetwork.fromFiles(TransportNetwork.java:223)
    at com.conveyal.r5.transit.TransportNetwork.fromDirectory(TransportNetwork.java:257)
    at com.conveyal.r5.point_to_point.PointToPointRouterServer.main(PointToPointRouterServer.java:79)
    at com.conveyal.r5.R5Main.main(R5Main.java:33)

To produce the scenario try building with the osm file in here that is titled Berkeley-no-service https://www.dropbox.com/sh/htfwroom0kt0khm/AADDYbB-y68O7q47j051LrkEa?dl=0

abyrd commented 7 years ago

Hi @zishanbilal. It looks like OSM data contains ways referencing nodes that are not included in the OSM data. We should probably modify R5 to ignore missing nodes but the simplest solution is to provide input data with referential integrity. Providing the completeWays=yes parameter to your Osmosis bounding-box operation will ensure that all referenced nodes are included in the clipped OSM file.

Actions to be taken on the R5 side:

  1. Add clear warnings when referenced nodes are missing.
  2. Recover gracefully from missing nodes, leaving undefined portions of the way out of the R5 network.
buma commented 7 years ago

This should probably also close #240

abyrd commented 7 years ago

Thanks for the hint @buma

buma commented 7 years ago

No problem. I just remembered that there is some similar issue between issues.

zishanbilal commented 7 years ago

I appreciate and even inspired by you quick response with solution. Thanks @abyrd

colinsheppard commented 7 years ago

As an addendum to this issue, though maybe a new but highly related issue, I found that if I filter certain classes of ways from OSM using OSMOSIS, the completeWays / completeRelations don't solve the integrity problem.

R5 ended up throwing a NullPointerException on this line:

https://github.com/conveyal/r5/blob/master/src/main/java/com/conveyal/r5/streets/StreetLayer.java#L695

Because there is a reference to a way in a turn restriction relation that was filtered out from an OSMOSIS command that looked like this:

osmosis --read-pbf file=california-latest.osm.pbf --bounding-box top=37.7256 left=-123.4232 bottom=36.9369 right=-121.6254 --tf reject-ways highway=service completeWays=yes completeRelations=yes --write-pbf file=sf-bay.osm.pbf

I've solved the issue by no excluding any way types, but this could come up for others in the future.

Thanks!

abyrd commented 7 years ago

Hi @colinsheppard, thanks for the report. Indeed we should also be checking for missing relation references. However I think this can again be remedied by using a different Osmosis command. The options completeWays=yes completeRelations=yes do not apply to the tag filter task, they must be applied to the bounding box task. I believe if you move them before --tf reject-ways highway=service you will get all referenced objects in your output PBF.

colinsheppard commented 7 years ago

Thanks Andrew. I actually ran the command correctly but copy/pasted incorrectly above (b/c I pasted the command in piecemeal). When I ran the filter / clipping this way, I still got the NP due to the turn restriction relation problem:

osmosis --read-pbf file=california-latest.osm.pbf --bounding-box top=37.7256 left=-123.4232 bottom=36.9369 right=-121.6254 completeWays=yes completeRelations=yes --tf reject-ways highway=service --write-pbf file=sf-bay.osm.pbf

Note that by running R5 in debug mode, I confirmed this issue, namely that it was looking for an edge that didn't exists because the OSM data (which I looked through as XML) had a turn restriction relation referring to a way that wasn't otherwise specified.

abyrd commented 7 years ago

I see. Yes, we need to make R5 robust when faced with incorrect OSM data which can occur even when it's not caused by geographic clipping.

mattwigway commented 7 years ago

@colinsheppard I pushed a fix for this which should hit master relatively soon. For the time being I'm guessing that the bad data might be because one of the referenced ways in the relation is tagged highway=service and is being rejected further down the pipe.

abyrd commented 7 years ago

The turn restriction problem should have been fixed in a205af8c1131c0814fe154b15dc6583b09f8890d yet it still happens. It appears that as @mattwigway said above, some ways are being filtered out further down the pipe after the relations are loaded and this is breaking the referential integrity. @demory has recently run into this on some data for the NY region.

abyrd commented 7 years ago

One thing is certain: this code is in severe need of modularization. I'm looking at an if clause that's 195 lines long.

abyrd commented 7 years ago

Hmm, thanks to a205af8c1131c0814fe154b15dc6583b09f8890d we're validating that every single member of the restriction is available in the OSM data before grabbing it. Yet fetching the "to" member is yielding a null pointer. I think the only way this can happen is if someone specified a node as the "to" member. It would then be validated as being present, but would be looked up as a way (since all from and to members of turn restrictions are supposed to be ways). I'll add a check for this.