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

Handle .mbtiles files #601

Merged
merged 13 commits into from Dec 11, 2018

Conversation

Projects
None yet
2 participants
@lewfish
Copy link
Contributor

lewfish commented Dec 4, 2018

Overview

This PR adds support for reading .mbtiles files (which store a set of vector tiles covering a region) in addition to zxy schemas for vector tiles. The same class is used for both -- it can just handle URIs for either type. Also:

  • Changes MBTilesVectorSource to VectorTileVectorSource which is a clearer name which reflects the fact it can handle vector tiles in different formats.
  • Adds a with_id_field option to handle the fact that the name of the id field for a feature varies across datasets.
  • Adds a reusable caching mechanism for downloaded files with the ability to handle gzipped files
  • Pins version of tippecanoe

Testing

  • The new functionality is covered by unit tests.
  • I also tested it using an experiment using the Belize mbtiles file which seemed to work.
  • I also tested it using the Spacenet Vegas example, but this did not work because the USA mbtiles file is 5GB and it runs too slowly. We will need a better caching mechanism to handle this use case. See azavea/raster-vision-examples#26 and #557

Closes #587
Closes azavea/raster-vision-examples#23

@lewfish lewfish added the review label Dec 4, 2018

@lewfish lewfish force-pushed the lf/mbtiles branch from f9381d7 to 541c3cc Dec 5, 2018

@lewfish lewfish changed the title WIP: Handle .mbtiles files Handle .mbtiles files Dec 5, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #601 into develop will increase coverage by 0.07%.
The diff coverage is 92.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #601      +/-   ##
===========================================
+ Coverage    71.99%   72.07%   +0.07%     
===========================================
  Files          170      170              
  Lines         7646     7674      +28     
===========================================
+ Hits          5505     5531      +26     
- Misses        2141     2143       +2
Impacted Files Coverage Δ
rastervision/registry.py 80.67% <ø> (ø) ⬆️
...on/data/vector_source/vector_tile_vector_source.py 100% <100%> (ø)
rastervision/data/vector_source/api.py 100% <100%> (ø) ⬆️
rastervision/data/vector_source/default.py 76.19% <50%> (ø) ⬆️
rastervision/rv_config.py 88.29% <75%> (-0.6%) ⬇️
rastervision/utils/files.py 93.47% <93.33%> (-0.03%) ⬇️
.../vector_source/vector_tile_vector_source_config.py 90.56% <93.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df5899b...32141b7. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #601 into develop will increase coverage by 0.06%.
The diff coverage is 92.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #601      +/-   ##
===========================================
+ Coverage       72%   72.07%   +0.06%     
===========================================
  Files          171      170       -1     
  Lines         7770     7674      -96     
===========================================
- Hits          5595     5531      -64     
+ Misses        2175     2143      -32
Impacted Files Coverage Δ
rastervision/registry.py 80.67% <ø> (ø) ⬆️
...on/data/vector_source/vector_tile_vector_source.py 100% <100%> (ø)
rastervision/data/vector_source/api.py 100% <100%> (ø) ⬆️
rastervision/data/vector_source/default.py 76.19% <50%> (ø) ⬆️
rastervision/rv_config.py 88.29% <75%> (-0.6%) ⬇️
rastervision/utils/files.py 93.47% <93.33%> (-0.03%) ⬇️
.../vector_source/vector_tile_vector_source_config.py 90.56% <93.33%> (ø)
rastervision/command/train_command_config.py 81.81% <0%> (-0.8%) ⬇️
rastervision/command/predict_command_config.py 81.01% <0%> (-0.7%) ⬇️
rastervision/command/chip_command_config.py 77.88% <0%> (-0.57%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a1d3c7...9115d9a. Read the comment docs.

@lewfish lewfish force-pushed the lf/mbtiles branch from 32141b7 to b54f620 Dec 5, 2018

@lewfish lewfish requested a review from jamesmcclain Dec 5, 2018

@lewfish

This comment has been minimized.

Copy link
Contributor Author

lewfish commented Dec 6, 2018

Note to self: I need to update the changelog for this (and the previous PR on vector sources). I will also update the PR template checklist to include an item for updating the changelog.

@jamesmcclain
Copy link
Member

jamesmcclain left a comment

Preliminary code comments in place. Behavioral testing to come.

raise rv.ConfigError(
'MBTilesVectorSourceConfigBuilder requires uri containing '
'{z}/{x}/{y}.')

if self.config.get('zoom') is None:

This comment has been minimized.

Copy link
@jamesmcclain

jamesmcclain Dec 7, 2018

Member

What if the uri does not conform to either schema? If this question has an obvious answer such as "a non-xyz uri could be anything", then that should perhaps be stated somewhere.

I am going commit-by-commit, so this question might be dealt with later. Comment is in reference to this commit in case it isn't clear.

This comment has been minimized.

Copy link
@lewfish

lewfish Dec 10, 2018

Author Contributor

Yes, it could be anything if it's not a ZXY schema. The with_uri method has a comment to this effect. If that's insufficient and you had something else in mind let me know.

@@ -49,6 +49,7 @@ RUN rm -f /usr/bin/python && ln -s /usr/bin/python3 /usr/bin/python
RUN cd /tmp && \
git clone https://github.com/mapbox/tippecanoe.git && \
cd tippecanoe && \
git checkout 9c708a && \
make && \

This comment has been minimized.

Copy link
@jamesmcclain

jamesmcclain Dec 7, 2018

Member

I've been meaning to say this in reference to the tensoflow models repo in as well, but maybe we should consider downloading zip files rather than cloning and checking it. The size delta (in terms of bytes downloaded and docker image size) is significant.

In reference to this commit.

This comment has been minimized.

Copy link
@lewfish

lewfish Dec 10, 2018

Author Contributor

Good idea.

@jamesmcclain
Copy link
Member

jamesmcclain left a comment

I also tested it using an experiment using the Belize mbtiles file which seemed to work.

Please be explicit.

@lewfish lewfish force-pushed the lf/mbtiles branch 2 times, most recently from 1437427 to 3ecaee9 Dec 10, 2018

@lewfish lewfish merged commit fb58fdc into develop Dec 11, 2018

@lewfish lewfish deleted the lf/mbtiles branch Dec 11, 2018

@lewfish lewfish removed the review label Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.