Skip to content

rasterize: honor resolution= exactly when bounds don't divide evenly#2597

Merged
brendancol merged 5 commits into
mainfrom
issue-2573
May 29, 2026
Merged

rasterize: honor resolution= exactly when bounds don't divide evenly#2597
brendancol merged 5 commits into
mainfrom
issue-2573

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2573.

Summary

  • rasterize(..., resolution=R) used to compute width/height via ceil(extent / R) but rebuilt output coords over the original bounds, so the actual cell size shrank to fit. bounds=(0,0,1,1), resolution=0.3 returned a 4x4 raster with cell size 0.25 instead of 0.3.
  • After resolving width/height, final_bounds is rebuilt so the grid anchors at (xmin, ymax) and extends right / down by exactly width * x_res / height * y_res. Matches rasterio.transform.from_origin(west, north, x_res, y_res).
  • When the original bounds already divide evenly, the new values equal the originals to float precision, so existing callers see no change.

Backend coverage

Fix lives upstream of backend dispatch, so all four backends pick it up: numpy, cupy, dask+numpy, dask+cupy. Cross-backend parity is pinned by the new test class.

Test plan

  • New regression test for the headline 4x4 / 0.25 example
  • Divides-evenly case still passes (no behavior change)
  • Asymmetric (x_res, y_res) tuple with non-divisible bounds
  • Polygon burn within original bounds is preserved when the grid expands
  • Cross-backend parity (numpy / cupy / dask+numpy / dask+cupy)
  • Full existing rasterize test suite passes (505 tests)

…2573)

Width and height were computed via ceil(extent / resolution), but output
coords were rebuilt over the original bounds.  Result: bounds=(0,0,1,1)
with resolution=0.3 returned a 4x4 raster with cell size 0.25, not 0.3.

Fix: after resolving final_width / final_height, rebuild final_bounds so
the grid anchors at (xmin, ymax) and extends right / down by exactly
width * x_res / height * y_res.  Matches the rasterio.transform.from_origin
convention.  Existing callers with bounds that divide evenly see no
change because width * x_res == xmax - xmin to float precision.

Tests cover the headline 4x4 / 0.25 regression, the divides-evenly
no-regression case, asymmetric (x_res, y_res) tuples, polygon burning
within the original bounds with the expanded grid, and cross-backend
parity (numpy / cupy / dask+numpy / dask+cupy).
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: rasterize resolution= fix (PR #2597)

Blockers

None.

Suggestions

  • xrspatial/tests/test_rasterize_resolution_exact_2573.py: no test combines like= with resolution= against a template whose bounds don't divide evenly. The existing test_resolution_override_strips_stale_grid_attrs at test_rasterize.py:2096 uses resolution=0.5 on a 10x10 template, so bounds divide evenly and the new overshoot path isn't exercised. A test with like= (10x10 template) plus resolution=0.3, asserting the output cell size is 0.3, would close that gap.
  • xrspatial/rasterize.py:3239-3241: the inferred-bounds + resolution path also picks up the fix. When a GeoDataFrame's total_bounds supplies non-divisible bounds and the caller passes resolution=, the grid now extends past the geometry envelope to honor the cell size. No test pins this. A small case with an irregular geometry, no explicit bounds=, and resolution=0.3 would lock in the inferred-bounds branch alongside the explicit-bounds branch.

Nits

  • xrspatial/rasterize.py:3239-3241: worth pinning the get_dataarray_resolution(result) side-effect. After the fix, that helper reports the cell size the caller actually asked for. A one-line assert against it in the existing test documents the downstream win for slope / aspect / proximity, which previously inherited the shrunk cellsize.

What looks good

  • Only the resolution= branch is touched. Explicit width/height and like=-only paths keep their behavior byte-for-byte.
  • Anchoring at (xmin, ymax) matches rasterio.transform.from_origin, which is what GDAL / rasterio users will expect.
  • width * x_res == xmax - xmin to float precision when bounds divide evenly is why the existing tests still pass without modification. The inline comment and CHANGELOG capture this.
  • Cross-backend parity is pinned for numpy, cupy, dask+numpy, and dask+cupy, since the fix lives upstream of dispatch.
  • Docstring update tells users what to expect when bounds don't divide evenly and points at the matching rasterio API. Better than silently changing behavior.
  • 505 existing rasterize tests pass unchanged.

Checklist

  • Algorithm matches the rasterio.transform.from_origin convention
  • All four backends produce consistent results
  • NaN / fill semantics unchanged
  • Edge cases covered: divides-evenly, doesn't-divide-evenly, asymmetric tuple, geom inside original bounds with expanded grid
  • No dask chunk boundary concerns; the change is in bounds resolution, not in the rasterize kernels
  • No new materialization or copies
  • [n/a] No new public function, so no benchmark or README feature matrix entry needed
  • Docstring updated for the new contract

…taarray_resolution tests (#2573)

Three follow-up tests prompted by /review-pr on #2597:

- TestLikePlusResolutionOvershoot: like= + non-divisible resolution=
  with the like-template bounds at 0..10 and resolution=0.3.  Existing
  test_resolution_override_strips_stale_grid_attrs used resolution=0.5
  which divides evenly, so the overshoot path under like= was unpinned.

- TestInferredBoundsPlusResolution: no explicit bounds= (geometry
  envelope drives final_bounds) plus resolution=0.3.  Pins the
  inferred-bounds branch separately from the explicit-bounds branch.

- test_get_dataarray_resolution_matches_request: asserts the helper
  reads back the requested cell size (0.3, not the pre-fix shrunk
  value 0.25).  Documents the downstream win for slope / aspect /
  proximity callers that prefer the coord-derived cellsize.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review (follow-up): rasterize resolution= fix (PR #2597)

Re-reviewed after c7eeacad. All three prior findings are now addressed.

Disposition of prior findings

  • Suggestion 1 (like= + non-divisible resolution=): fixed by TestLikePlusResolutionOvershoot (10x10 template + resolution=0.3 -> 34x34 grid with exact cell size).
  • Suggestion 2 (inferred-bounds + resolution=): fixed by TestInferredBoundsPlusResolution (no explicit bounds=, geometry envelope drives final_bounds).
  • Nit (get_dataarray_resolution side-effect): fixed by test_get_dataarray_resolution_matches_request.

Blockers

None.

Suggestions

None.

Nits

None.

12/12 regression tests pass. Existing 505-test rasterize suite still green. No further findings.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 28, 2026
@brendancol brendancol merged commit e1b81c7 into main May 29, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rasterize: resolution= argument not honored when bounds don't divide evenly

1 participant