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

Test break because difference in glacierareas map #143

Closed
JoostBuitink opened this issue Jan 10, 2023 · 6 comments
Closed

Test break because difference in glacierareas map #143

JoostBuitink opened this issue Jan 10, 2023 · 6 comments
Labels
bug Something isn't working
Milestone

Comments

@JoostBuitink
Copy link
Collaborator

Automated test break, because of a difference in the glacierareas map in the staticmaps. It appears that an change altered how a vector (or GeoDataFrame) is rasterized onto the model grid. Is potentially caused by an upstream change in rasterio.rasterize: but I couldn't find an clear change in their code.

See figure below, showing the results of rasterizing a glacier GeoDataFrame onto the model grid, the difference is in the red outline. Left shows the results created with rasterio 1.2.10 (and hydromt 0.6.0 and hydromt_wflow 0.2.1), right with rasterio 1.3.4 (and the master branches from hydromt and hydromt_wflow)

image

Note that the glacier geometry does indeed cover the topright pixel (glacier geometry in grey, and the green area is the pixel that is no longer considered to be a glacier with raster 1.3.4)

image

@hboisgon
Copy link
Contributor

Thanks for checking @JoostBuitink. Seems gdal made a 3.6.0 (from 3.5.*) and rasterio 1.3.4 was released around the same time to support gdal 3.6+.
If it's a change in GDAL not really sure if we can do so much about it...
So just update our test example staticmaps to reflect the change?
@DirkEilander would you agree?

@DirkEilander
Copy link
Contributor

The point is that with the new results seem wrong as we use the "ALL_TOUCHED=TRUE" options of gdal rasterize. Perhaps good to investigate if this is a (known) bug in the new version or that the argument is no long in use or ... If it's a (known) bug it might be better to ignore the error (or temporarily pin to an older gdal version) until it is fixed. Otherwise we'll need to change the map back once it is fixed.

@hboisgon
Copy link
Contributor

I tried to look if the error was mentioned already, gdal already released two versions with fixes for 3.6 but could not find anything about rasterize and all_touched mentioned yet. Maybe we could raise the issue indeed?

@DirkEilander
Copy link
Contributor

I also can't find anything. To raise the issue we first need to make sure that this behavior is reproducible with just rasterio (or just gdal rasterize) by creating a minimal reproducible example of the error. We could create this with a clip of the glaciers layer and a wflow map.

@hboisgon hboisgon added this to the Release 0.3.0 milestone Mar 1, 2023
@hboisgon
Copy link
Contributor

hboisgon commented Mar 9, 2023

For now we update the glaciers map and hope this will be solved later

@JoostBuitink
Copy link
Collaborator Author

It appears in the latest version of rasterio (assuming the problem was here), this is fixed again. Tests in #123 were failing on the glacier maps, with the same issue as presented here (but then inversed). In this PR, I reverted the glacier map, as this is expected behavior with all_touched=True. This way, the tests can also be completed successfully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants