Skip to content

Running through the readme sequentially results in a warning #68

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

Closed
tmcw opened this issue Apr 13, 2018 · 9 comments
Closed

Running through the readme sequentially results in a warning #68

tmcw opened this issue Apr 13, 2018 · 9 comments

Comments

@tmcw
Copy link

tmcw commented Apr 13, 2018

Currently, if you run through the label-maker readme example - with the togo country config.json, and running in order:

label-maker download
label-maker labels
label-maker preview -n 10
label-maker images
label-maker package

The last step produces a warning for a missing file:

Saving QA tiles to data/togo.mbtiles
   100%     26.6 MiB       1.9 MiB/s            0:00:00 ETA
Retiling QA Tiles to zoom level 12 (takes a bit)
174704 features, 10870707 bytes of geometry, 4 bytes of separate metadata, 2597853 bytes of string pool
  99.9%  12/2060/1976
Determining labels for each tile
---
Roads: 11 tiles
Buildings: 9 tiles
Total tiles: 12
Writing out labels to data/labels.npz
Writing example images to data/examples
Downloading at most 10 tiles for class Roads
Downloading at most 10 tiles for class Buildings
Downloading 11 tiles to data/tiles
Couldn't open data/tiles/2063-1978-12.jpg, skipping
Saving packaged file to data/data.npz

Admittedly I haven't done enough digging yet, but it would look like there's possibly an off-by-one error here.

@tmcw
Copy link
Author

tmcw commented Apr 13, 2018

Looks like this is #63

@tmcw tmcw closed this as completed Apr 13, 2018
@jreiberkyle
Copy link
Contributor

jreiberkyle commented Apr 13, 2018

Hmm.. I wonder if this is truly related to #63? I get that skipping error as well on my slightly altered run through of the README here. I chocked it up to being due to tiles that don't contain either of the classes not being downloaded. Could you check with previewing just 9 or 8 tiles (making sure you clear out the preview directories)?

@drewbo
Copy link
Contributor

drewbo commented Apr 16, 2018

@jreiberkyle yup, this is due to some lazy packaging scripts (it searches for an image for each tile even though, depending upon the config parameters, we don't download an image for each tile). Because the image download in the previous step is partially randomized (for background tiles), I think one of two approaches makes the most sense:

  1. Keep the code as is, drop the warning
  2. Glob the images directory and iterate over that instead of the full label/tile set

Any preferences? @jreiberkyle @tmcw @wronk

@drewbo drewbo reopened this Apr 16, 2018
@wronk
Copy link
Contributor

wronk commented Apr 16, 2018

Do we want to support users to directly manipulate images directory (by adding or removing examples)? It seems like we might want (2) if that's the case. I'd also lean towards always alerting users to any mismatches.

Counter question to myself on (2): are there any risks of /images not matching up with /labels? Would that confuse users if we use a file list directly from /images but they expect something else based on their input to the labels command?

@drewbo
Copy link
Contributor

drewbo commented Apr 17, 2018

@wronk I think both options will create the same output in most cases, even if users manually edit the /images directory (in the first case, we skip the image, in the second, we never glob it)

@wronk
Copy link
Contributor

wronk commented Apr 17, 2018

Maybe the second one makes more sense then? I really don't have any strong opinions here, so whatever you think will make it easier for the user is okay with me.

@jreiberkyle
Copy link
Contributor

(it searches for an image for each tile even though, depending upon the config parameters, we don't download an image for each tile).

@drewbo in which instances are image tiles not downloaded? Is this only the case for background tiles?

@drewbo
Copy link
Contributor

drewbo commented Apr 17, 2018

@jreiberkyle yup, either all background tiles or just a random portion of them (depending upon the background_ratio property).

I'm going to implement option 1 (drop the warning) and assume that everything in the images folder is what people want (if there were download issues, it will surface while downloading). If there are corrupt files (i.e. OSError as opposed to FileNotFoundError) then we can show a warning and skip (and probably resolve #43 in the process)

@jreiberkyle
Copy link
Contributor

Sounds great @drewbo !

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

No branches or pull requests

4 participants