-
Notifications
You must be signed in to change notification settings - Fork 108
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
improve cutline handling #598
Conversation
ef56d10
to
add3e5a
Compare
For the docs, it seems like aside from the Either an extension of https://cogeotiff.github.io/rio-tiler/examples/Using-rio-tiler-mosaic/ or a fresh one similar to the snipped in the ticket above. |
add3e5a
to
e93b796
Compare
e93b796
to
ac0da5b
Compare
👍 all good for me. Would you be able to write a note in the changelog 🙏 |
ac0da5b
to
8dfa736
Compare
Added changelog note |
214a1a3
to
a6e70d9
Compare
One sidenote: the create_cutline utils function is no longer used in this codebase. What to do in this case? Just leave it be, or deprecate? |
@yellowcap why is the |
I first left it in, but the problem is that the cutline mask is done in the projection of the source image, so it won't exactly align with the cutline mask done in the target projection. The "finish early" will fail in some of these cases because there are pixels that are masked in the original projection, but not anymore in the target one. So these pixels will always be masked in the target projection. I thought about this in terms of what it means for data transfer efficiency. My guess is that this will not have implications for data transfers, but not sure. Maybe some COG internal tile requests can be skipped with the cutline vrt_option in place. Do you know if GDAL optimizes that under the hood, i.e. identifying what tiles not to get based on the cutline? |
Also, I just realized that the masking will not fully work for rasters with multiple resolutions (10m vs 20m) in this case. So I'll have to digg into that, the stop early fails when multiple resolutions are included. |
I added another commit that solves the multiple resolution problem. The approach is to only apply the same cutline mask to all assets using the highest resolution rasterization. |
It might (really doubt) but I don't think this will be a major issueQ 👍 Let's keep |
0894fad
to
093b9cb
Compare
rio_tiler/models.py
Outdated
for img in data: | ||
if img.height == max_h and img.width == max_w: | ||
continue | ||
arr = numpy.ma.MaskedArray( | ||
resize_array(img.array.data, max_h, max_w), | ||
mask=resize_array(img.array.mask * 1, max_h, max_w).astype("bool"), | ||
mask=cutline_mask, |
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.
🤔 what happens if there is no cutline_mask
in the ImageData object? I think this line shouldn't be changed
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.
You are absolutely right again! But that opens the problem that pixels might never get unmasked when upsampling. In the following example, the green pixel would always be masked at the coarse resolution, but at the higher resolution it would not be masked. So the finish early would not work again. The solution is to go back to what you already suggested earlier - setting always all_touched
to True
. In that case the finer mask should always be included in the coarser mask. Will make change accordingly.
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.
👍 we could also add a mechanism to set an option for finish early
when like 95% of the cutline is filled 🤷
093b9cb
to
00cc7da
Compare
@@ -551,7 +549,7 @@ def feature( | |||
[shape], | |||
out_shape=(img.height, img.width), | |||
transform=img.transform, | |||
all_touched=all_touched, | |||
all_touched=True, # Necesary for matching masks at different resolutions |
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.
👍
@yellowcap all seems good to me now! do you want to add a tests for the |
00cc7da
to
42c0943
Compare
ok test for |
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.
We need to handle projection difference between the input feature and output crs
thanks a ton @yellowcap for working on this 🙏 |
Awesome! Thanks for all the help and getting it over the last humps @vincentsarago ! |
* update readme * PER_BAND mask and MaskedArray (#580) * sketch use of MaskedArray with PER_BAND mask * update PointData and fix tests * update mosaics * update nodata mask * remove unused * update changelog * migration guide * split resampling option to RasterIO and Warp options (#590) * refactor Mask tests and Benchmark * refactor Mask tests and Benchmark (#591) * change arguments in dynamic_tiler.md (#592) * change arguments The argument ("tile", {"z": "{z}", "x": "{x}", "y": "{y}"}) causes errors below. File "/home/ubuntu/app/app.py", line 48, in tilejson tile_url = request.url_for("tile", {"z": "{z}", "x": "{x}", "y": "{y}"}) TypeError: HTTPConnection.url_for() takes 2 positional arguments but 3 were given ("tile", z='{z}', x='{x}', y='{y}') is correct. * Update docs/src/advanced/dynamic_tiler.md --------- Co-authored-by: Vincent Sarago <vincent.sarago@gmail.com> * refactor MosaicMethods (#594) * refactor MosaicMethods * fix tests * Optional boto3 (#597) * make boto3 an optional dependency * update migration * update test dependencies * add flake8 rules in ruff (#600) * update dev version * update mosaic example notebook custom pixel_selection class (#602) * improve cutline handling (#598) * Improve cutline handling Closes #588 * handle projection --------- Co-authored-by: vincentsarago <vincent.sarago@gmail.com> * remove useless cache * save benchmarks * add benchmarking in docs * fix nodata/mask/alpha forwarding for GCPS dataset (#604) * remove boto3 * Allow clip-box to auto expand (#608) * Allow clip-box to auto expand * Add test * Update tests/test_io_xarray.py * Fix import order * Changes from black linter * Change default to true * use auto_expand in part and update changelog --------- Co-authored-by: Vincent Sarago <vincent.sarago@gmail.com> * allow morecantile 4.0 (#606) * allow morecantile 4.0 * set morecantile min version to 4.0 * update changelog * forward statistics from STAC raster:bands (#611) * forward statistics from STAC raster:bands * forward tags as metadata and forward metadata when creating ImageData from list * update changelog * handle nodata in XarrayReader (#612) * handle nodata in XarrayReader * update changelog * add AWS credential overrides for S3 stac (#613) * add AWS credential overrides for S3 stac * catch warnings * deprecated model methods (#615) * update readme --------- Co-authored-by: TTY6335 <36815385+TTY6335@users.noreply.github.com> Co-authored-by: Daniel Wiesmann <yellowcap@users.noreply.github.com> Co-authored-by: Aimee Barciauskas <aimee@developmentseed.org>
This is another attempt to resolve #588
The proposed changes ensure that the
FirstMethod
finishes when all pixels over the target geometry is filled.The main implementation challenge is: the
pixel_selection
class needs to know the target mask to use inis_done
function.This means that the input geometry needs to be converted to a mask as numpy array to which the
pixel_selection
needs to have access.For this case, the cutline
vrt_option
approach does not work, because for gdal cutlines the mask generation happens on the gdal side and the mask is not returned. So the proposed changes do not use thevrt_option
argument, but rasterized the geometry directly and applies this mask on the python side after requesting pixels.