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

create_cutline expose rasterio rowcol op parameter to fix pixel raste… #759

Merged

Conversation

Martenz
Copy link
Contributor

@Martenz Martenz commented Oct 25, 2024

create_cutline expose rasterio rowcol op parameter to fix pixel raster x,y approximation

Related issue: #758

@vincentsarago
Copy link
Member

thanks @Martenz
Would you be able to add tests 🙏

@Martenz
Copy link
Contributor Author

Martenz commented Oct 25, 2024

thanks @Martenz Would you be able to add tests 🙏

It will take some time from my side

Martino added 3 commits October 25, 2024 13:17
…lue. TODO: next need to test with specific feat and round or ceiling operators
…not one of the accepted methods by rasterio.transform.rowcol
@Martenz
Copy link
Contributor Author

Martenz commented Oct 25, 2024

Hi, I added the operator parameter to the existing unit tests, and I also added a test to check and raise an exception if the operator is not one of the accepted methods for the rasterio.transform.rowcol function. This means the cutline is now tested against previous implementations, allowing future users to specify something other than math.floor to obtain XS, YS pixel values if needed.

As for testing my specific issue (#758), I'm not entirely sure how to proceed. I think the problem arises because the mask is the exact contour line of a group of pixels, and with the applied approximations, the default math.floor operator primarily selects the pixels on the left side, with a few pixels inside the mask. This happens regardless of any adjustments to vrt_options. Using the round operator seems to be the only solution for accurate pixel selection in this case.

To test this behavior, I might add a fixture with a cog.tif file and a small feature mask that matches the exact pixel boundaries. However, I'm unsure about what exactly to assert in this case.

Anyhow just to double check I run all test also replacing the operator op to all test with "round" and they pass successfully 100%.
Would this solve a note I found in the code tests/test_io_rasterio.py#L764 ?

Apologies if my unit testing skills are lacking, and thank you for your support.

rio_tiler/utils.py Outdated Show resolved Hide resolved
rio_tiler/utils.py Outdated Show resolved Hide resolved
src_y, src_x = rowcol(src_dst.transform, xs, ys)
polygon = ", ".join([f"{x} {y}" for x, y in list(zip(src_x, src_y))])
src_y, src_x = rowcol(src_dst.transform, xs, ys, op=op)
polygon = ", ".join([f"{int(x)} {int(y)}" for x, y in list(zip(src_x, src_y))])
Copy link
Member

Choose a reason for hiding this comment

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

we cast x and y to int to avoid rasterio/rasterio#3219

geometry_crs="epsg:4326",
op=np.ceil,
)
assert cutline_npceil != cutline_npfloor
Copy link
Member

Choose a reason for hiding this comment

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

added specific tests to show the difference between operators

rio_tiler/utils.py Outdated Show resolved Hide resolved
@vincentsarago
Copy link
Member

@Martenz made some edits to your PR, hope you're 👍

I'll wait for your 👍 before merging

@vincentsarago vincentsarago merged commit b833783 into cogeotiff:main Oct 29, 2024
8 checks passed
@Martenz
Copy link
Contributor Author

Martenz commented Oct 29, 2024

Thanks @vincentsarago for your support on this, and sorry for my late reply. I think is fine as far as I can use the round now. I will test it tomorrow.

@Martenz
Copy link
Contributor Author

Martenz commented Oct 30, 2024

Works great as tested. Thanks again, I hope to contribute more.

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.

2 participants