Add special handling when processing cut coord z16 tiles #407
Conversation
peitili
changed the title
peitili
changed the title
tilequeue/process.py
Outdated
| @@ -556,16 +562,56 @@ def format_coord( | |||
| # the output. | |||
| def process_coord(coord, nominal_zoom, feature_layers, post_process_data, | |||
| formats, unpadded_bounds, cut_coords, buffer_cfg, | |||
| output_calc_spec, scale=4096, log_fn=None, max_zoom_with_changes=16): | |||
| output_calc_spec, scale=4096, log_fn=None, | |||
There was a problem hiding this comment.
I stared at this line for like 10 seconds before I realized there was no change
There was a problem hiding this comment.
We should run our autoformatter on the whole repo
There was a problem hiding this comment.
sorry, it was an unintended change, but it does pass the linter too. anyway, I reverted the change.
| # If our highest supported zoom is not 16, the system might be | ||
| # broken, so we assert 16 here. Plus, we hardcoded 16 elsewhere too: | ||
| # https://github.com/tilezen/tilequeue/blob/43a4d4d1b101a4410660c23f1d41222e85aaa3ba/tilequeue/process.py#L382 | ||
| assert max_zoom_with_changes == 16 |
There was a problem hiding this comment.
if this works, I think we can actually do something more like
if max_zoom_with_changes == nominal_zoom:
and then base everything off of that. Like right now this breaks for anyone with an alternate config
There was a problem hiding this comment.
In fact I intentionally use the hardcode value because of the comment I added above this line.
In short, we don't know whether it shall work when max_zoom_with_changes is not 16, when it is not 16, it may not throw exception throughout the whole process, but maybe the result is logically wrong; and if this happens it is way harder to debug what's wrong since the code doesn't error out and the bug is hidden.
Thus, we want to error out here by doing an assertion and postponing the generalization to future development. When people do want to let this handling work for non-16 value in the future, they will encounter this error here and they know what they are doing and will test and take care of the result's correctness by correcting the assertion then.
There was a problem hiding this comment.
In other words, I don't think we need to spend time testing the result of max_zoom_with_changes != 16 now in this PR.
There was a problem hiding this comment.
I'm fine keeping the hard coded 16 here, as we hard code 16 a few other places and its easier to read.
But the below section will likely significantly increase the build time, because it's doing a repeat pass in cutting the tiles. Let's see the actual impact in a build.
There was a problem hiding this comment.
We did a mini build and the logic does produce the correct results.
tilequeue/process.py
Outdated
| # formated tiles that processed with hacked bumped nominal zoom 17 | ||
| # as the final result | ||
| all_formatted_tiles_cut_coord_z15_z16.extend( | ||
| [ft for ft in all_formatted_tiles_special if ft.get('coord').zoom == 16]) |
There was a problem hiding this comment.
does the order of these formatted tiles matter? If not, couldn't you combine this into one comprehension?
There was a problem hiding this comment.
does the order of these formatted tiles matter? If not, couldn't you combine this into one comprehension?
I am not sure whether it matters, but I think it is better to keep the order as before since the order is deterministic. In the for loop, lower zoom will be handled first and then higher zooms and this is the original behavior.
There was a problem hiding this comment.
A few PRs depend on this change, namely tilezen/vector-datasource#2052 and https://github.com/tilezen/vector-datasource/pull/2056/files.
Depending on impact on global build processing time we can optimize the last step.
There is an issue that zoom 15, 16 mvt tiles shares a nominal zoom 16, and that nominal 16 will be used to check against the end_zoom config value in the queries.yaml file. (this issue not only happens for nominal zoom 16 but also happens for nominal zoom 0, in a word, the edge cases happens on both ends but is OK for the middle zooms) With this issue, when we write end_zoom: 17 in the queries.yaml, both zoom 15 and 16 mvt files will be affected; if we write end_zoom: 16, only zoom 14 will be affected. So zoom15 and zoom16 mvt files have to be operated together or none.
The current tilequeue implementation only passes the nominal zoom to vector-datasource for those post_process functions defined in queries.yaml, then it cut coords to generate mvt files, it doesn’t pass the actual mvt file coord as a parameters when checking against vector-datasource. i.e. when nominal zoom is 16 it calls vector-datasource once with only 16, then it cut coords to generate zoom15 and zoom16 tiles.
One possible solution without too much refactoring I can think of is: when nominal is 16 at tilequeue, call vector-datasource twice.
Then select the cut zoom15 tiles from step 1 and select the cut zoom16 tiles from step 2 and concatenate them to return as the result for nominal zoom 16.
Obviously, this solution 2x the processing time for these tiles.