Skip to content

geotiff: surface rotated_affine 6-tuple on DataArray attrs (#2129)#2157

Merged
brendancol merged 2 commits into
mainfrom
issue-2129
May 20, 2026
Merged

geotiff: surface rotated_affine 6-tuple on DataArray attrs (#2129)#2157
brendancol merged 2 commits into
mainfrom
issue-2129

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2129.

Summary

  • Reads opened with allow_rotated=True now emit attrs['rotated_affine'] as a rasterio-style 6-tuple (a, b, c, d, e, f) so callers can recover the rotated mapping without reaching into geo_info.transform.
  • The attr only appears when the source carried a rotated ModelTransformationTag; plain no-georef reads and axis-aligned reads do not grow the attr.
  • Read-only contract: attrs_to_metadata (the writer-side boundary parser) intentionally drops rotated_affine, so to_geotiff keeps writing a plain no-georef file until the writer learns to emit ModelTransformationTag (allow_rotated does not work for rotated GeoTIFFs #2115 follow-up).
  • Bumps the attrs contract version from 3 to 4; existing keys keep their pre-v4 shape.

Backend coverage

All four read backends share the same marshalling step (_populate_attrs_from_geo_infogeo_info_to_metadatametadata_to_attrs), so numpy, cupy, dask+numpy, and dask+cupy paths all emit the new attr without per-backend code.

Test plan

  • test_rotated_affine_attr_2129.py covers the rotated path (with and without CRS), the plain no-georef path, axis-aligned reads, tuple type guarantee, dask path, and the writer round-trip drop contract.
  • Existing test_allow_rotated_* suites still pass — CRS-drop and no-georef behaviour unchanged.
  • Contract-version pin tests in test_attrs_contract_version_1984.py and test_georef_status_2136.py updated to track v4.
  • Full xrspatial/geotiff/tests/ suite: 4453 passed, 1 pre-existing unrelated failure on test_lowlevel_write_pushdown_2138::test_write_vs_to_geotiff_byte_parity_uint8[lz4] (also fails on main).

Reads opened with ``allow_rotated=True`` now emit
``attrs['rotated_affine']`` as a rasterio-style 6-tuple so callers
can recover the rotated mapping without reaching into reader
internals. The attr is read-only: ``to_geotiff`` keeps dropping the
rotation on round-trip until the writer learns to emit
``ModelTransformationTag`` (#2115 follow-up).

Bumps the attrs contract version to 4. Existing keys keep their
pre-v4 shape; only the additive ``rotated_affine`` is new.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 20, 2026
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: geotiff: surface rotated_affine 6-tuple on DataArray attrs (#2129)

Blockers

None.

Suggestions

  • VRT backend does not emit rotated_affine for the same condition. xrspatial/geotiff/_backends/vrt.py:277 detects rotated GDAL GeoTransforms via _vrt_is_rotated = (gt[2] != 0.0 or gt[4] != 0.0) and routes the array into the rotated_dropped bucket on both the eager (line 336) and chunked (line 842) builds. Neither GeoTIFFMetadata(...) site populates the new rotated_affine field, so a VRT user who opens a rotated VRT with allow_rotated=True still cannot recover the 6-tuple. Same gap #2129 closes for the ModelTransformationTag path. The 6-tuple is reconstructable from vrt.geo_transform via the existing _gdal_geotransform_to_affine_tuple helper (used by the non-rotated VRT branch at line 597). Adding rotated_affine=_gdal_geotransform_to_affine_tuple(vrt.geo_transform) if _vrt_is_rotated else None to both builds keeps the backends consistent.

Nits

  • test_geotiff_metadata_2139.py::_representative_attrs_dicts (line 237) drives the symmetric round-trip tests. The new attr is intentionally asymmetric (read-only), so it does not belong there — but a short comment near the helper saying so would prevent a future contributor from adding it and hitting a confusing failure.
  • No GPU-backed end-to-end test for the new attr. The GPU read path shares the marshalling step with the eager/dask paths so the unit tests cover the logic, but a @_gpu_only sibling would close the matrix.

What looks good

  • Flows through the existing GeoTIFFMetadata / metadata_to_attrs step rather than adding a parallel code path.
  • Read-only round-trip contract is explicit and pinned: attrs_to_metadata does not parse the attr back, and test_attrs_to_metadata_drops_rotated_affine will fail loudly if that ever changes.
  • tuple(src_t.rotated_affine) cast is defensive against a parser change to list/ndarray; test_rotated_affine_is_tuple_not_list pins the type.
  • Contract version bump from 3 to 4 is paired with both pin-test updates and an attrs_contract.rst versioning note.
  • test_open_geotiff_axis_aligned_omits_rotated_affine is a useful negative test: an axis-aligned ModelTransformationTag still rides on attrs['transform'] and the new attr stays off.

Checklist

  • Algorithm matches reference (N/A — metadata)
  • All implemented backends consistent (caveat: VRT gap above)
  • NaN handling correct (N/A)
  • Edge cases tested (rotated with/without CRS, plain no-georef, axis-aligned, dask, tuple type, writer-side drop)
  • Dask chunk boundaries handled (test_open_geotiff_rotated_emits_rotated_affine_dask)
  • No premature materialization
  • Benchmark exists or not needed (not needed)
  • README feature matrix updated (N/A — no new function)
  • Docstrings updated (open_geotiff + attrs_contract.rst)

Closes the cross-backend gap surfaced in the PR review: the VRT
backend already detects rotated GDAL GeoTransforms via
``_vrt_is_rotated`` and lands the array in
``georef_status='rotated_dropped'``, but neither
``GeoTIFFMetadata(...)`` build site populated the new
``rotated_affine`` field. A rotated VRT opened with
``allow_rotated=True`` now surfaces the rotated 6-tuple alongside
the rotated TIFF path, reconstructed via the existing
``_gdal_geotransform_to_affine_tuple`` helper.

Also adds:

* eager + chunked VRT tests, plus an axis-aligned VRT negative test,
  to ``test_rotated_affine_attr_2129.py``;
* a docstring note on ``_representative_attrs_dicts`` explaining
  why ``rotated_affine`` is intentionally excluded from the
  symmetric ``metadata_to_attrs`` / ``attrs_to_metadata`` round-trip
  fixture.
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 pass on a4c48d5

Blockers

None.

Suggestions

None.

Nits

None.

What looks good

  • VRT rotated_affine gap closed at both build sites (_backends/vrt.py:343 eager, _backends/vrt.py:854 chunked); _gdal_geotransform_to_affine_tuple correctly maps GDAL ordering to rasterio Affine ordering, so the VRT surface matches the non-VRT ModelTransformationTag surface.
  • The new _vrt_rotated_affine is computed only when _vrt_is_rotated; gating mirrors the existing rotated_dropped gate so the two attrs stay in lockstep.
  • VRT tests added (eager + chunked + axis-aligned negative) and the symmetric round-trip helper now carries an inline note explaining why rotated_affine is excluded — so a future contributor does not waste cycles wondering.
  • Lazy import of _gdal_geotransform_to_affine_tuple is already in scope at both modified sites (existing imports at lines 231 and 603).

Disposition of original review

  • Suggestion (VRT gap): fixed in a4c48d5.
  • Nit (_representative_attrs_dicts doc note): fixed in a4c48d5.
  • Nit (GPU-marked test): dismissed. The GPU read path shares _populate_attrs_from_geo_info with eager/dask, which test_rotated_optin_emits_rotated_affine_tuple already exercises directly. A GPU-marked sibling would only confirm the shared helper still runs under cupy, which the four other GPU rotated tests in the suite already cover at the attrs['crs'] / attrs['transform'] level.

Nothing left to fix.

@brendancol brendancol merged commit 31c1b92 into main May 20, 2026
5 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.

geotiff: surface rotated_affine 6-tuple on the DataArray attrs

1 participant