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

[Needs review] add zoom range "min..max" options for burn + other changes #26

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

vincentsarago
Copy link
Contributor

@vincentsarago vincentsarago commented Jan 25, 2019

👋 @dnomadb
I think having the possibility to pass a zoom range to the burn command could be a nice feature (at least for myself). I tried to apply the same logic at in rio-mbtiles where we pass the Min Max zoom as a min..max string.

rio bounds my_file.tif | supermercado burn 18..21 | xt -d'-' > list_tiles.txt

I know this could be achieve with a bash onliner but you know I don't love bash 😄

I also did some other change to improve code readability and help.

I'll work on the tests as soon if we agree on this options

cc @sgillies

@vincentsarago
Copy link
Contributor Author

👋 @dnomadb I've updated the code and added a custom zoom type to make sure the input is 👌.
I've also updated the tests but sadly they are failing due to a depreciation in numpy inside findedges function

  File /edge_finder.py", line 26, in findedges
    )), axis=2) - burn)
TypeError: numpy boolean subtract, the `-` operator, is deprecated, use the bitwise_xor, the `^` operator, or the logical_xor function instead.

@dnomadb dnomadb mentioned this pull request Dec 13, 2019
Copy link

@1z2x3c4v5b6n7m8 1z2x3c4v5b6n7m8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

G7

Copy link

@1z2x3c4v5b6n7m8 1z2x3c4v5b6n7m8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for t in tiles:
click.echo(t.tolist())
tiles = (
burntiles.burn(features, zoom) for zoom in range(zoom[0], zoom[1] + 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to re-burn for every zoom; we can use the maxzoom to generate the tile-cover, then:

tiles[:, :2] >>= 1
tiles[:, 2] -= 1
np.unique(tiles, axis=0)

This will output the parents of tiles. cc @pratikyadav

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain @dnomadb ? Does that not risk false positives, sweeping in small child tiles that are outside the features?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgillies y of course!

A feature that covers a tile at zoom z will always cover said tiles at zooms z-n. False positives would be a (huge) risk going to higher zooms but if we perform the tile cover at the maxzoom of the provided range, we can then derive lower zooms directly from this array.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Thanks, I must have had some min/max zoom confusion in my head at the end of the day.

Base automatically changed from master to main March 10, 2021 21:33
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

Successfully merging this pull request may close these issues.

5 participants