-
Notifications
You must be signed in to change notification settings - Fork 19
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
add support for preprocessed inline geojson layers #414
Conversation
for more information, see https://pre-commit.ci
…n this case you intended it to be used
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
tilequeue/process.py
Outdated
@@ -286,6 +290,12 @@ def process_coord_no_format( | |||
# filter, and then transform each layer as necessary | |||
for feature_layer in feature_layers: | |||
layer_datum = feature_layer['layer_datum'] | |||
# inline layers are expected to be pre-processed | |||
layer_path = layer_datum.get('layer_path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cross comment:
tilezen/vector-datasource#2095 (comment)
tilequeue/process.py
Outdated
features = [] | ||
|
||
# has to exist and has to be geojson for now | ||
if not os.path.isfile(layer_path) or not layer_path.endswith('.geojson'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any log we can emit to tell whether it happens, otherwise it is hard to debug if it not working as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we specifically dont want to do that because the layer is completely optional. so it will always appear in queries.yaml but its up to the runner of the pipeline to supply the file. if they dont supply it we just ignore it
with open(layer_path) as fh: | ||
fc = json.load(fh) | ||
# skip if this isnt pseudo mercator | ||
if fc['crs']['properties']['name'] != 'urn:ogc:def:crs:EPSG::3857': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern, is there any log we can emit to tell whether it happens, otherwise it is hard to debug if it not working as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we can log as it was clear the person meant to load it but screwed something up. maybe same for the file extension
tilequeue/process.py
Outdated
|
||
# add the features geometries with their properties (values must be strings) in tuples | ||
for geom, feat in itertools.izip(gc.geoms, fc['features']): | ||
properties = {k: str(v) if type(v) is not bool else str(v).lower() for k, v in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use another name for v
in the for k,v
expression to increase readability?
@@ -645,7 +683,7 @@ def convert_source_data_to_feature_layers(rows, layer_data, unpadded_bounds, zoo | |||
# expecting downstream in the process_coord function | |||
for layer_datum in layer_data: | |||
layer_name = layer_datum['name'] | |||
layer_props = row_props_by_layer[layer_name] | |||
layer_props = row_props_by_layer.get(layer_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any side-effect this might introduce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think so. if you scroll up just a bit you'll see that this object has props for every layer except this new landmarks layer. those are set to either a value or to None
. now there isnt one for landmarks so changing this to get
will just add handling for landmarks not existing above. alternatively i could have added landmarks above and just set it to None
but there are a lot of places in the code that say something like "TODO: stop hardcoding all the layers" so i didnt want to add to the mess.
tilequeue/command.py
Outdated
@@ -2176,6 +2177,8 @@ def tilequeue_meta_tile(cfg, args): | |||
# each coord here is the unit of work now | |||
pyramid_coords = [job_coord] | |||
pyramid_coords.extend(coord_children_range(job_coord, zoom_stop)) | |||
# for brevity of testing it is sometimes convenient to reduce to a single child coord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, say single child cood (here over XYZ placename)
just what the title says. you can now modify queries.yaml to reference layers from mercator projected geojson files. the way in which ive implemented this is fairly lightweight in that we dont support transforms or filters on the initial data (ie we dont load any yaml files like water.yaml buildings.yaml etc) because we expect the layer to be pretty much already in its final format. this does mean however that postprocessing transforms are allowed since they dont have any filtering or column juggling.
this pr is a corollary to the one over here: tilezen/vector-datasource#2095