search

mapbox / tile-cover

Generate the minimum number of tiles to cover a geojson geometry
MIT License
189 stars 40 forks source link

Replace min_zoom option with just merge #70

Closed mourner closed 9 years ago

mourner commented 9 years ago

After #51, the overhead to running merge over 1 zoom level compared to merge over all zoom levels is very little. For example:

scan russia - z0 - z10 x 12.37 ops/sec ±3.27% (11 runs sampled)
scan russia - z9 - z10 x 12.68 ops/sec ±5.10% (11 runs sampled)
scan russia - z10 - z10 x 23.11 ops/sec ±5.37% (12 runs sampled)

Given that, does it make sense to keep min_zoom configurable? We could replace it with one option — merge — that would indicate whether there's any merging happening at all or not, and it would always happen up to z0 since there's no overhead and this makes the API simpler and reduces the amount of decisions you have to make when using it.

If we add a merge option, we can as well rename max_zoom to zoom and so address #68 as well.

@morganherlocker created a new issue since you didn't answer the question two previous times (https://github.com/mapbox/tile-cover/pull/51#issue-48705984 and https://github.com/mapbox/tile-cover/issues/49#issuecomment-62962537).

morganherlocker commented 9 years ago

I think there are legitimate reasons to cap the min_zoom depending on the use case. I can imagine database applications in which you would end up with certain low zoom index-spaces being flooded with features. I think we should keep the flexibility here.

mourner commented 9 years ago

Makes sense. I think we should at least make min_zoom default to max_zoom if not specified.