BUG: Fix rio.bounds() for rotation / asymmetric shear in the transform (#847)#931
Open
Leonard013 wants to merge 1 commit into
Open
BUG: Fix rio.bounds() for rotation / asymmetric shear in the transform (#847)#931Leonard013 wants to merge 1 commit into
Leonard013 wants to merge 1 commit into
Conversation
corteva#847) `_affine_has_rotation` previously returned `affine.b == affine.d != 0`, which only detected symmetric shear. For an asymmetric shear/rotation (`b != d`, either nonzero) it returned False, so the transform was treated as north-up and `rio.bounds()` ignored the shear. Fixing the detection alone is not enough: `bounds()` never computed an axis-aligned envelope for a rotated grid, so even the symmetric-shear case (where detection already returned True) produced incorrect bounds. This: - widens `_affine_has_rotation` to `affine.b != 0 or affine.d != 0` (rotation or shear, symmetric or not), and - computes `bounds()` as the envelope of the four transformed grid corners when the transform has rotation, matching `rasterio`'s `DatasetReader.bounds` for a non-north-up transform. The north-up fast path is unchanged. Adds a parametrized regression test covering asymmetric shear, symmetric shear and a single off-diagonal, cross-checked against rasterio's own bounds. This change was written with the assistance of Claude (AI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #847
Why
rioxarray/_spatial_utils.py::_affine_has_rotationreturned:That only detects symmetric shear (
b == d). For an asymmetricshear/rotation (
b != d, either nonzero) it returnsFalse, so the grid istreated as north-up:
_resolutionreturns(a, e)andrio.bounds()ignores theshear entirely.
While confirming the bug I found the problem is actually broader than detection.
bounds()never computes an axis-aligned envelope for a rotated grid — it pads thecoordinate/origin extent by the (sign-lost) resolution. So even the
symmetric-shear case (where
_affine_has_rotationalready returnedTrue)produced wrong bounds. Both paths were incorrect; only the symptom differed.
Reproduce (
w=4, h=5):rio.bounds()(before).bounds/ 4-corner envelope (correct)Affine(1, 0.3, 0, 0.1, -1, 0)asymmetric(0, -5, 4, 0)(0, -5, 5.5, 0.4)Affine(1, 0.3, 0, 0.3, -1, 0)symmetric(0, 0, 4.18, 5.22)(0, -5, 5.5, 1.2)Affine(1, 0.2, 0, 0, -1, 0)single off-diagonal(0, -5, 4, 0)(0, -5, 5, 0)(Patching only
_affine_has_rotationchanges the asymmetric result to(0, 0, 4.02, 5.22)— still wrong, now matching the broken symmetric path.)What
_affine_has_rotationtoaffine.b != 0 or affine.d != 0(rotation orshear, symmetric or not). This mirrors rasterio's own
not (b == d == 0)test inDatasetReader.bounds._rotated_bounds(affine, *, width, height)returning the axis-alignedenvelope of the four transformed grid corners
affine * {(0,0),(w,0),(0,h),(w,h)}→(min x, min y, max x, max y).bounds()uses that envelope when the (cached) transform has rotation; thenorth-up fast path is untouched.
Geometry reasoning
For a north-up transform the outer extent is just the opposite corners, so the
existing resolution-based path is fine. Under rotation/shear the grid is no longer
axis-aligned, so the outermost coordinates are the envelope of all four corners —
exactly what
rasterio.io.DatasetReader.boundsreturns for a non-north-uptransform. Corners are computed from the pixel-corner transform (as written to the
GeoTransform), so the result is edge (not center) bounds, consistent with the
north-up path. Verified equal to rasterio's
DatasetReader.boundsto <1e-9 forasymmetric, symmetric, single-off-diagonal, and north-up transforms.
Side effect on
resolution()(deliberate)Widening
_affine_has_rotationalso routesresolution()for asymmetric /single-off-diagonal grids through the rotated branch, so it now returns magnitudes
(sqrt(a²+d²), sqrt(b²+e²))instead of(a, e). This is intentional and bringsthose (rare) grids in line with rasterio's
resconvention; for symmetric shearthis was already the behavior. No test encoded the old
(a, e)value.Tests
test_bounds__rotated(asymmetric shear, symmetric shear, singleoff-diagonal): asserts
_affine_has_rotationisTrueand thatrio.bounds()equals both the 4-corner envelope and rasterio's own
DatasetReader.bounds. Failsbefore, passes after.
reproject_matchtests intest_integration_rioxarray.pystill pass (58 passed). No existing expected valueschanged (the previously-broken symmetric-shear bounds were not asserted anywhere).
Note re: upstream
There is a parallel discussion about rasterio (rasterio#2586). Per @snowman2's
guidance on #847, this is fixed in rioxarray — @lantson confirmed rasterio
already computes these bounds correctly (
DatasetReader.bounds), and this PR bringsrio.bounds()in line with it. No upstream change is required.Prepared with AI assistance (Claude Code): the reproduction, root-cause analysis
(including that the symmetric-shear path was also broken), fix, and tests were
AI-drafted; the geometry reasoning and rasterio-parity verification are documented
above.