Skip to content

Expand zonal_statistics supported geometry types#450

Merged
davemfish merged 6 commits intonatcap:mainfrom
megannissel:feature/448-zonal-stats-poly-types
Dec 15, 2025
Merged

Expand zonal_statistics supported geometry types#450
davemfish merged 6 commits intonatcap:mainfrom
megannissel:feature/448-zonal-stats-poly-types

Conversation

@megannissel
Copy link
Copy Markdown
Contributor

@megannissel megannissel commented Dec 10, 2025

Fixes #448

zonal_statistics previously only supported wkbPolygon and wkbMultiPolygon geometries. However, it would be reasonable to support 3D and measured (multi)polygons as well, as the z coordinates shouldn't have an affect / should be ignored for the purposes of the zonal stats calculations.

This change also brings zonal_statistics in line with InVEST validation of input vectors, where the model spec input geometry_types maps 'POLYGON' and 'MULTIPOLYGON' to several ogr types:

'POLYGON': [ogr.wkbPolygon, ogr.wkbPolygonM,
            ogr.wkbPolygonZM, ogr.wkbPolygon25D],
'MULTIPOLYGON': [ogr.wkbMultiPolygon, ogr.wkbMultiPolygonM,
                 ogr.wkbMultiPolygonZM, ogr.wkbMultiPolygon25D]

Allowing for the same list of valid geometries will prevent InVEST models that calculate zonal_statistics from failing on vectors that would otherwise be valid InVEST inputs.

@megannissel megannissel marked this pull request as draft December 10, 2025 19:16
@megannissel megannissel marked this pull request as ready for review December 11, 2025 22:06
@davemfish davemfish self-requested a review December 11, 2025 22:13
Copy link
Copy Markdown
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

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

@megannissel this looks good to me.

As far as the tests, I think testing all the different geometry types is probably not necessary. It certainly doesn't hurt, and if it was useful to write the tests in order to convince ourselves that all the different types are treated the same way, then that's a good enough reason to write the tests. But our function here does not do anything different with the different types, so I think we're ultimately testing things like gdal.Rasterize more than we're testing pygeoprocessing.zonal_statistics. And in that sense, I don't think we need to test gdal.

Comment thread tests/test_geoprocessing.py Outdated

polygon_a = [
(origin[0], origin[1]),
(origin[0], -pixel_size * n_pixels+origin[1]),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a really minor thing, but I think these would be more readable with spaces around the + operator. Especially since that operator is lower priority than the * which does have spaces. Alternatively, no spaces around * and spaces around + would also be more readable, I think.

@megannissel
Copy link
Copy Markdown
Contributor Author

Writing these tests and confirming that nothing unexpected happened did make me feel more confident that I wasn't overlooking anything. But completely agree that we don't really need to be testing gdal itself.

Copy link
Copy Markdown
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @megannissel . I forgot to suggest that we update HISTORY, but I think we should do that too.

@davemfish davemfish merged commit a2432c7 into natcap:main Dec 15, 2025
93 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.

Be less strict about polygon geometry types in zonal statistics

2 participants