Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shapefile import via geotools #616

Closed
wants to merge 4 commits into from
Closed

Shapefile import via geotools #616

wants to merge 4 commits into from

Conversation

vvikas
Copy link
Contributor

@vvikas vvikas commented Dec 22, 2015

Shape file reader :

  • Abstract shapefile reader
  • Implementation for OSM shape files
  • Basic test case on small sample shapefile
  • graphhopper.sh modifications

Run it via : ./graphhopper.sh web - asia_india.shp

@@ -22,6 +22,7 @@
<netbeans.hint.license>apache20</netbeans.hint.license>
<maven.build.timestamp.format>yyyy-MM-dd'T'HH:mm:ssZ</maven.build.timestamp.format>
<builddate>${maven.build.timestamp}</builddate>
<geotools.version>10.8</geotools.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use a currently supported version of Geotools, eg. 14.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had used, mvn clean test verify
that seemed to use java 1.6 hence had to use previous version.
Here tests are run against 1.7 and 1.8 , so will update to newer version.

@karussell
Copy link
Member

Hi Vikas,

thanks for this PR! Shapefiles are very valuable, especially to import or handle proprietary data. Is there a special format expected or certain attributes? E.g. how is the speed value per vehicle guessed? Or will it just import the topology and handling other attributes will be done elsewhere?

In order to integrate this we need to do some work like #450. This is already planned and creates a separate osm-import module. Then we need an additional refactoring separating OSM stuff from FlagEncoders. See also the discussion and the available refactoring in #277, maybe there are even some valuable refactorings which can be used here? Maybe @engaric can comment on this?

Recently I had the pleasure with playing with shp files on my own (maybe I'll blog about this) and geotools is just too huge to be included in the normal distribution or even in core. But after these refactorings we should be able to put the geotools stuff into a separate module like shp-import which then provides an implementation of DataReader, identical to what then OSM will provide.

The unit tests looks good. Additionally we need a stronger validation here e.g. ensuring that certain networks exists and the location lookup works, plus a simple Dijkstra run.

Run it via : ./graphhopper.sh web - india-latest.osm

Do you mean india-latest.shp here instead? When doing the import: how large is the shp file and how much RAM is used for the import? Also after the necessary refactorings the shp files should work directly like osm or pbf files without the dash:
./graphhopper.sh web india-latest.shp

@vvikas
Copy link
Contributor Author

vvikas commented Dec 22, 2015

Yes ! ./graphhopper.sh web - asia_india.shp

@karussell karussell changed the title 0.5 Shapefile import via geotools Dec 22, 2015
@vvikas
Copy link
Contributor Author

vvikas commented Dec 22, 2015

I have abstracted out the logic to handle shape file handling and building graph.
And processJunctions and proccessRoads methods need to be implemented by the
ShapeFile reader depending on the data.
One implementation I have provided for, is for files at : http://download.geofabrik.de
Different vendors have different schemes, who can create their shape file reader by implementing above methods.

Vehicle speeds are not explicitly calculated. For now just using 'maxspeed' in shape file, if not present an Average Speed is used. This will need improvements, which will again depend on the "schema of shape file".

@devemux86
Copy link
Contributor

Interesting implementation and useful too for proprietary data.

we should be able to put the geotools stuff into a separate module like shp-import

👍
Like Peter mentioned, it's better to not not mix the various import modules.
A core module and extensions like the osm-import or shp-import with their own dependencies is probably the best way.

@vvikas
Copy link
Contributor Author

vvikas commented Dec 23, 2015

@karussell there is gc overhead in one build, should increase the maven memory ?

@karussell
Copy link
Member

And processJunctions and proccessRoads methods need to be implemented by the
ShapeFile reader depending on the data. One implementation I have provided for, is for files at : http://download.geofabrik.de

Ah, okay!

How do you determine if something is a junction vs. a bridge?

Vehicle speeds are not explicitly calculated. For now just using 'maxspeed' in shape file, if not present an Average Speed is used. This will need improvements, which will again depend on the "schema of shape file".

Perfect!

@karussell there is gc overhead in one build, should increase the maven memory ?

Hmmh, what do you mean here? I was asking about memory usage just because of comparison to OSM importer.

Regarding the refactoring: I'll try to implement #450 in the next weeks and then it shouldn't take much to change this PR to be merged. What do you think?

@vvikas
Copy link
Contributor Author

vvikas commented Dec 23, 2015

Hmmh, what do you mean here? I was asking about memory usage just because of comparison to OSM importer.

Please check attached log for asia_india.shp , for zip of ~300 MBs with RAM of 1G took about 5-6 minutes.
import.log.zip

The memory usage I was taking about was in the 'Tarvis CI build' , one of its test instance is failing due to maven OOM.

I will take at look at #450 and other issues and see how much is to be done (refactoring,redesign, etc for ShapeFileReader module) and commit as per availability.

@karussell
Copy link
Member

Just to let you know: this is not forgotten :) As this is important work I'm working on #450 in a new branch and will provide this soonish as a new pull request. Then this work needs to be refactored (into a new maven module) to integrate it better in the new structure.

@karussell karussell added this to the 0.8 milestone Jun 12, 2016
@karussell
Copy link
Member

karussell commented Jun 12, 2016

@vvikas would you have a bit time to see if an integration of this PR is possible (e.g. as new module reader-shp) with the new separation done in #740?

@vvikas
Copy link
Contributor Author

vvikas commented Jul 4, 2016

Great , changes looks good. Will try to integrate with it. Should be done by end of July.

@karussell
Copy link
Member

Thanks a lot for the feedback! Will then try to integrate this in the next weeks.

@karussell
Copy link
Member

BTW: Merged the changes so this one here could be better separated too

@@ -22,6 +22,7 @@
<netbeans.hint.license>apache20</netbeans.hint.license>
<maven.build.timestamp.format>yyyy-MM-dd'T'HH:mm:ssZ</maven.build.timestamp.format>
<builddate>${maven.build.timestamp}</builddate>
<geotools.version>13.2</geotools.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

You will want to update this to the latest maintainance release (14.x). The current stable (15.x requires java 8)

Copy link
Member

Choose a reason for hiding this comment

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

As this won't be in the core we can use java 8 like we do with the web module too.

@karussell karussell removed this from the 0.8 milestone Oct 10, 2016
@karussell
Copy link
Member

Adaptions to this PR done in #874

@karussell karussell closed this Nov 16, 2016
karussell pushed a commit that referenced this pull request Nov 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants