Refactored OSM Shapefile importer #874

PGWelch · 2016-11-16T13:40:24Z

Fixes / changes completed

Highway tag – we’re now using this (not used before, previously the code was using a hardcoded speed or max speed if non-zero).
Max speed tag now passed properly to the encoders. Added a filter for value “0” as in Geofabrik shapefiles 0 means no tag, but 0 will probably just mean 0 speed in the Graphhopper flag encoders…
One way tags now used properly. Previously the code was only checking for “0” and treating this as boolean. You also have to check for against the direction of digitisation (e.g. value “-1” if its one-way against the directory of digitisation). Also Geofabrik is using an odd (unfortunately non-standard) convention for their shapefile oneway field; I now read this convention but turn into the standard convention in the ReaderWay I pass to the flag encoders. This could break in the future if Geofabrik clean this up (the code will throw an exception with a useful message if it breaks).
Encoding manager now used properly. I now fill a ReaderWay with the tags and pass to the EncodingManager (the EncodingManager was not used previously), so we should get proper behaviour over multiple speed profile types (car, bike etc). Previously there was some hardcoding to “car”.
Fixed partially connected graph bug. With a shapefile from Geofabrik we don’t have node objects, only the geometry of the road (they don’t have a separate node export shapefile). Worse still, the road geometry generally isn’t split at junctions / intersections. Previously the code was assuming that any point at the start or the end of a road geometry line needed to be a node, and was splitting edges wherever one of their geometry points corresponded to the start or end of another edge. This fails badly though, because many times you get two edges crossing each other (e.g. at crossroads) where neither edges are broken at the crossroads – resulting in edges not being split at the crossroads, the graph being only half-connected and hence some pretty odd routes. Instead I’ve changed the code to create a node – and therefore split edges – based on any identical coordinate shared in two or more edges. This is (I believe) the best we can do with Geofabrik shapefile data but may fail in some odd / rare circumstances (e.g. as no z-level considered). This also means each coordinate (pillar or tower) gets stored in a hashmap during the build, which is not memory-friendly for very large files. However for a HERE importer I believe the data would be setup differently (so we’d be able to do this differently).
Edge length was being rounded down to int metres, I’ve kept it in double instead.
Added the estimated_distance and estimated_centre artificial tags
Ensured code handles shapefile multilinestring or linestring geometry (both are possible, previously multilinestring was assumed).
Unit tests - two tests for travel times, comparing between building using pbf (the existing reader project) and the shapefile project. One test is between some predefined points with known travel times, the other chooses many pairs of random points (200 currently). For the random pairs of points, we’re getting a mean agreement in travel times between the pbf and shp of around 99%, which is pretty good as the shapefile isn’t taking into account lots of other tags : ) Obviously in some cases we get less agreement presumably due to tags we’re not using…
Unit tests - we now have a (very basic) geometry test
Unit tests - we now have a test for a single one way example
Small amounts of code tidying (no code conventions applied though).

Improves #616

karussell · 2016-11-16T18:42:32Z

Thanks, will review!

karussell

Looks good, had only some minor notes. Will test now with real data.

karussell · 2016-11-16T18:38:57Z

reader-shp/.gitignore

@@ -0,0 +1,2 @@
+#Don't ignore my Malta test data used in the automated test!!
+!malta-latest.osm.pbf 


Would you mind to move this to the root .gitignore?

Changed in the next commit.

karussell · 2016-11-16T18:40:17Z

reader-shp/pom.xml

+    <name>GraphHopper Reader for Shapefile Data</name>
+
+    <properties>      
+        <geotools.version>13.2</geotools.version>


As commented here #616 (comment) we could upgrade to 15.x (?)

Updated to 15.2 in next commit.

karussell · 2016-11-16T18:41:13Z

reader-shp/pom.xml

+                </configuration>
+            </plugin>
+        </plugins>
+    </build>


This section can be removed?

Changed in the next commit.

karussell · 2016-11-16T18:42:09Z

reader-shp/src/main/java/com/graphhopper/reader/shp/GraphhopperShp.java

+import com.graphhopper.reader.shp.OSMShapeFileReader.EdgeAddedListener;
+import com.graphhopper.storage.GraphHopperStorage;
+
+public class GraphhopperShp extends GraphHopper {


I know it is a bit nitpicking but would you mind to rename to GraphhopperSHP as we have GraphHopperOSM

Also would you add the GraphHopper license header to all new files?

Changed in the next commit. Probably not gramatically correct though! OSM is an acronymn, SHP isn't. Licence file added.

karussell · 2016-11-16T18:43:31Z

reader-shp/src/main/java/com/graphhopper/reader/shp/OSMShapeFileReader.java

+ * OSMShapeFileReader for files present at : http://download.geofabrik.de/ It extracts the data as per the structure of
+ * shape files
+ *
+ * @author Vikas Veshishth, Philip Welch


Here also a minor edit:

@author Vikas Veshishth @author Philip Welch

Changed in the next commit.

karussell · 2016-11-16T20:21:15Z

reader-shp/src/main/java/com/graphhopper/reader/shp/OSMShapeFileReader.java

+
+			}
+		} catch (Exception e) {
+			LOGGER.error("Exception while processing for roads: ", e);


Here we have to throw an exception, e.g. throw new RuntimeException("some info", e)

Changed in the next commit. I've changed the exceptions to be unchecked (as I hate checked exceptions!) and don't catch them anymore (other than wrapping them in a runtime exception to uncheck them if needed). The original exception is likely to be more informative than a rethrown one anyway.

karussell · 2016-11-16T20:22:53Z

reader-shp/src/main/java/com/graphhopper/reader/shp/ShapeFileReader.java

+/**
+ * ShapeFileReader takes care of reading a shape file and writing it to a road network graph
+ *
+ * @author Vikas Veshishth


Feel free to add yourself here too :)

Changed in the next commit

karussell · 2016-11-16T20:51:13Z

reader-shp/src/main/java/com/graphhopper/reader/shp/GraphhopperShp.java

+		return initDataReader(reader);
+	}
+
+	public void addListener(EdgeAddedListener l) {


Did I understood the purpose of the listeners: adding custom attributes to the graph? Why are multiple listeners necessary?

The listeners don't modify anything, they're only used to help me output debugging information when checking over the building of the graph. They're not actually called from the commited code (just an external main I was using to run a debug from). They are useful though, so I'd prefer keep them in temporarily.

karussell · 2016-11-16T20:52:54Z

reader-shp/src/main/java/com/graphhopper/reader/shp/Utils.java

+import com.vividsolutions.jts.geom.GeometryFactory;
+
+public class Utils {
+	public static String toWKT(PointList list) {


Is it necessary to create a new factory for every call? Or is performance overhead neglectable and problematic stuff to ugly like non-thread safety?

This method is only called a couple of times a unit test (and some of my other debugging code like below which I've left out of the project). Performance overhead is negligible.

karussell · 2016-11-16T20:54:08Z

reader-shp/src/test/java/com/graphhopper/reader/shp/ShapeFileReaderTest.java

+			new File(tempOutputDirFromShp).mkdirs();
+			new File(tempOutputDirFromPbf).mkdirs();
+
+			System.out.println("Building from shapefile");


No logging necessary in the tests IMO

Prefer to keep the printlns if OK. There's some console output from the tests I like to check : )

The problem is that if every test does this we would be overwhelmed by log statements (we have >1K tests ;)). And every test should just print ok to command line if succeeded. For development purposes you can use this (and throw away afterwards) although I always recommend to formulate the things you check on command line with your eyes into an assert statement. Do you know what I mean :) ?

karussell · 2016-11-16T21:18:30Z

When trying 'sachsen' I get:

Caused by: java.lang.NullPointerException
    at com.graphhopper.reader.shp.ShapeFileReader.getFeatureIerator(ShapeFileReader.java:56)
    at com.graphhopper.reader.shp.OSMShapeFileReader.processJunctions(OSMShapeFileReader.java:82)

Will look into it tomorrow.

PGWelch · 2016-11-17T09:37:19Z

Re: sachsen. I've just tried this file and its fine. Route built below using this code.

GraphhopperSHP gh = new GraphhopperSHP();
gh.setWayPointMaxDistance(0);
gh.setStoreOnFlush(false)
        .setDataReaderFile(
                "C:\\Users\\Phil\\Dropbox\\Business\\Companies\\Graphhopper\\Shapefile importer\\Sachsen-dataset\\gis.osm_roads_free_1.shp")
        .setGraphHopperLocation(new File("c:\\temp").getAbsolutePath())
        .setEncodingManager(new EncodingManager(new CarFlagEncoder())).setCHEnabled(false).importOrLoad();

GHPoint dresden = new GHPoint(51.0504 ,13.7373);
GHPoint leipzig = new GHPoint(51.3397,12.3731);

GHRequest request = new GHRequest(dresden,leipzig).setVehicle("car");
GHResponse response = gh.route(request);
if(!response.hasErrors()){
    System.out.println(Utils.toWKT(response.getBest().getPoints()));

}

The exception suggests the shapefile can't be loaded? You've unzipped and referenced the roads file yes?

karussell · 2016-11-17T09:47:58Z

Works now surprisingly well - thanks a bunch!

The exception suggests the shapefile can't be loaded? You've unzipped and referenced the roads file yes?

I specified the folder not the shp file ... was too late yesterday ;) ... we should still add a NPE check (?)

karussell · 2016-11-17T12:10:27Z

It looks like geotools is not JDK9 ready, which is a bit ugly:

java.lang.IllegalArgumentException: org.geotools.data.DataStoreFactorySpi is not an ImageIO SPI class
    at com.graphhopper.reader.shp.ShapeFileReaderTest.initHopper(ShapeFileReaderTest.java:118)
    at com.graphhopper.reader.shp.ShapeFileReaderTest.setupBeforeClass(ShapeFileReaderTest.java:131)

Do you have an idea if we somehow can workaround this without changing geotools? This was the only thing I found in the web but maybe this need a change to geotools directly: levigo/jbig2-imageio#18

karussell · 2016-11-17T12:19:24Z

Ah, it is already known to them: https://osgeo-org.atlassian.net/browse/GEOT-5289 and here geotools/geotools#1064

PGWelch · 2016-11-17T13:34:17Z

hmm.. Probably best to wait until they've fixed it then.

Dr Philip WelchOpen Door Logistics LimitedSpecialists in open source solutions for transport logistics.

Email: info@opendoorlogistics.com
Linkedin: http://uk.linkedin.com/in/welchphilip
Skype: opendoorlogistics
Tel: +44 (0) 20 8144 4009
Website: http://www.opendoorlogistics.com

---- On Thu, 17 Nov 2016 12:19:27 +0000 Peter<notifications@github.com> wrote ----

Ah, it is already known to them: https://osgeo-org.atlassian.net/projects/GEOT/issues/GEOT-5289?filter=allopenissues
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

karussell · 2016-11-17T15:14:08Z

For now I've just excluded the shp module from JDK9 build and will still merge.

karussell · 2016-11-17T15:30:06Z

Merged it via this commit 1960b0f

The ugliness here is that I wanted it in a single commit (for simplicity) but now it looks like a change only from vvikas. Still when you look at 'git log' the author is "vvikas and [your name]" although it did not pick up the second email, obviously. I hope this is okay and have added you as a contributor in our list and hopefully more changes are coming :) (?)

PGWelch · 2016-11-17T15:36:03Z

Cool. Don't worry about who the change shows up from - it's not a problem.

PGWelch added 5 commits November 11, 2016 16:51

Refactored shapefile importer - needs testing!

0b8a52b

test passing

8de9441

sub gitignore so i can get test data into git

39d2133

Malta data

bde9333

formatting, % deviation for test

a8a2b51

karussell mentioned this pull request Nov 16, 2016

Shapefile import via geotools #616

Closed

karussell added the new feature label Nov 16, 2016

karussell added this to the 0.9 milestone Nov 16, 2016

karussell reviewed Nov 16, 2016

View reviewed changes

PGWelch added 4 commits November 17, 2016 10:28

Misc changes described in comments of PULL request

6931d43

commit failed on GraphhopperSHP.. recommiting.

651ace0

Rename GraphhopperShp.java to GraphhopperSHP.java

1cbef6d

added reader-shp to parent pom

6d851b5

karussell pushed a commit that referenced this pull request Nov 17, 2016

OSM Shapefile importer #616 and #874

1960b0f

karussell closed this Nov 17, 2016

karussell mentioned this pull request Apr 6, 2020

Move reader-shp into separate repository #1484

Merged

		@@ -0,0 +1,2 @@
		#Don't ignore my Malta test data used in the automated test!!
		!malta-latest.osm.pbf No newline at end of file

Refactored OSM Shapefile importer #874

Refactored OSM Shapefile importer #874

Uh oh!

Conversation

PGWelch commented Nov 16, 2016 • edited by karussell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karussell commented Nov 16, 2016

Uh oh!

karussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karussell commented Nov 16, 2016

Uh oh!

PGWelch commented Nov 17, 2016

Uh oh!

karussell commented Nov 17, 2016

Uh oh!

karussell commented Nov 17, 2016

Uh oh!

karussell commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PGWelch commented Nov 17, 2016

Uh oh!

karussell commented Nov 17, 2016

Uh oh!

karussell commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PGWelch commented Nov 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

PGWelch commented Nov 16, 2016 •

edited by karussell

Loading

karussell commented Nov 17, 2016 •

edited

Loading

karussell commented Nov 17, 2016 •

edited

Loading