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

Improved elevation tile downloading in Python #3318

Merged
merged 12 commits into from Nov 1, 2021

Conversation

chrstnbwnkl
Copy link
Contributor

Issue

fixes #3298 by replacing valhalla_build_elevation bash script with a corresponding Python script, which

  • adds shapely support while still supporting the bbox method
  • uses hyperthreading
  • accepts a directory path as an argument that will be parsed for GeoJSON files
  • handles Polygons and MultiPolygons

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Waiting to merge #3308 so this can be properly tested

@nilsnolde nilsnolde self-requested a review September 13, 2021 13:26
@nilsnolde
Copy link
Member

we already had some rounds internally reviewing this. until final review I'd ideally like to merge #3308 before. anyone chiming in is appreciated!

@nilsnolde nilsnolde changed the title Improved tile downloading in Python Improved elevation tile downloading in Python Sep 13, 2021
@nilsnolde
Copy link
Member

@chrstnbwnkl #3308 is merged, you can look there to see how testing works for these scripts: the most important part is that you relatively symlink the script you wanna test into the folder where the test file is with a .py extension, then you can import the script you wanna test easily

@@ -0,0 +1 @@
/Users/christianbeiwinkel/Documents/internship/dev/valhalla/scripts/valhalla_build_elevation
Copy link
Member

Choose a reason for hiding this comment

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

I made the same mistake, see #3308 (comment)

you can leave it for now until we had a chance to discuss it in the PR meeting (to save some CI hours:))

@nilsnolde
Copy link
Member

nilsnolde commented Oct 5, 2021

to leave it here as well: I just found a file which is not built anymore, https://github.com/valhalla/valhalla/blob/master/src/valhalla_tiles_for_elevation.cc. that actually outputs a list of the elevation tile "names" the graph covers. it seems to be forgotten about over the years and I think it doesn't need to be written in c++ as long as it's not actually lazy-loading at runtime as @kevinkreiser suggested but just a script producing output.

should we include that functionality in this PR and remove that file? I didn't think about it before but now that I see it it makes total sense. then we'd have 3 options for downloading elevation tiles: bbox, polygons and data-driven, all having their merit IMO.

@dnesbitt61
Copy link
Member

I wrote that a couple years ago as a way of reducing the elevation data size by not downloading data over oceans and places where no roads exist. This reduced the size of worldwide elevation from ~1.5TB to ~500GB. It would be great to add this logic to valhalla_build_elevation as an option.

@nilsnolde nilsnolde mentioned this pull request Oct 19, 2021
6 tasks
@nilsnolde
Copy link
Member

I added the option to only download tiles the graph covers to valhalla_build_elevation and re-arranged a little but didn't change much more @chrstnbwnkl . was in a bit of a hurry, we want to use that feature for the fossgis server, so we only download 500 gigs.

should be ready for review. CI will fail, we first need the docker image with python3-shapely installed. I'll PR real quick

@nilsnolde nilsnolde marked this pull request as ready for review October 31, 2021 20:12
@nilsnolde nilsnolde merged commit a694ae6 into valhalla:master Nov 1, 2021
ianthetechie pushed a commit to stadiamaps/valhalla that referenced this pull request Apr 24, 2022
* rewrote valhalla_build_elevation in python, added shapely support for improved tile downloading

* lock added to dir creation

* lock removed

* added first tests for valhalla_build_elevation

* fixed bad symlink to build_elevation script

* added output-only method: if path is specified, tile names are written to it without downloading tiles

* add option to download only tiles the graph covers; finalize tests

* change the --from-graph method to only look at level 2 tiles

* rename vars for clarity

Co-authored-by: nilsnolde <nils.nolde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elevation downloader in python?
4 participants