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

proposal/bugfix: Remove checks for gdal resampling algorithm values #388

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

emlys
Copy link
Member

@emlys emlys commented Apr 9, 2024

Fixes #387

This was pretty simple so I went ahead with it, but happy to consider alternatives too!

We were relying on a gdal bug to build a list of human-readable names for resampling algorithms. In gdal 3.8.5, this no longer works. We could implement a different way to get the names, but I propose that we remove the list entirely.

The lists in question (_GDAL_WARP_ALGORITHMS and _GDAL_WARP_ALGOS_FOR_HUMAN_EYES) are used in warp_raster and build_overviews. They're only used to check that the provided resampling algorithm is a valid option, and raise a helpful error message if not. I think we can do away with that because GDAL raises its own (slightly less helpful) error message if given an invalid algorithm name:

gdal.WarpRaster raises a TypeError: in method 'wrapper_GDALWarpDestName', argument 4 of type 'GDALWarpAppOptions *' and prints out ERROR 5: Unknown resampling method: <resampling method>. With gdal.UseExceptions(), you get a nicer RuntimeError: Unknown resampling method: <resampling method>.

gdal.BuildOverviews raises a RuntimeError: Building overviews failed or was interrupted for <raster path>. and prints out ERROR 1: GDALGetResampleFunction: Unsupported resampling method "<resampling_method>". With gdal.UseExceptions(), you get RuntimeError: GDALGetResampleFunction: Unsupported resampling method "<resampling method>".

These error messages aren't quite as nice as what pygeoprocessing had, but they're adequate to identify the issue. Since we pass the resample method argument directly to the underlying gdal function, I think it makes sense to let GDAL handle it.

A couple of behavior changes:

  • The gdal.WarpRaster resample algorithm argument can be a gdal.GRA_* constant as well as a string name. By removing the pygeoprocessing check, we can accept gdal.GRA_* constants too.
  • gdal.WarpRaster and gdal.BuildOverviews use the nearest neighbor algorithm if the resampling argument starts with 'near'. So 'near', 'nearest', 'nearest_neighbor', and 'near_foo' all work.

@emlys emlys requested a review from phargogh April 9, 2024 20:50
@emlys emlys self-assigned this Apr 9, 2024
@emlys emlys marked this pull request as ready for review April 9, 2024 20:53
Copy link
Member

@phargogh phargogh left a comment

Choose a reason for hiding this comment

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

Yeah, this makes sense as a solution. In GDAL 4.0 the switch to exceptions by default will make it more readable anyways, and this solution is a more general one since, as you point out, allows GDAL to decide how to interpret a warp mode and whether it is invalid. Thanks @emlys !

Just waiting to confirm that the tests pass before merging.

@phargogh phargogh merged commit c4a8830 into natcap:main Apr 9, 2024
92 checks passed
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.

GDAL warp algorithm names missing with GDAL>=3.8.5
2 participants