From 5cfacfe5850d4c4c68395c56b7ccbd68f5dff3a2 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 07:54:55 -0700 Subject: [PATCH 1/4] geotiff: drop crs/crs_wkt when allow_rotated drops the transform (#2126) ``_populate_attrs_from_geo_info`` wrote ``crs`` / ``crs_wkt`` before the ``has_georef`` check, so the ``allow_rotated=True`` opt-in path dropped the transform but kept the CRS attrs. The public docstring for ``open_geotiff`` already advertised that CRS is dropped on rotation; the implementation lagged. Downstream code that gates on ``"crs" in da.attrs`` treated the array as spatially meaningful while the axis-aligned transform was gone. Gate the CRS writes on the rotated opt-in detection: skip them when ``transform.rotated_affine`` is set and ``has_georef`` is False. Plain no-georef files (no transform tags at all) still keep their CRS attrs -- only the rotated path drops them, matching the docs. Updated the ``allow_rotated`` docstring in ``__init__.py`` to spell out that both ``crs`` and ``crs_wkt`` are dropped, and corrected the old reference to ``geo_info.transform.rotated_affine`` which is not accessible from the returned DataArray. Closes #2126 --- xrspatial/geotiff/__init__.py | 14 +- xrspatial/geotiff/_attrs.py | 39 +++-- .../tests/test_allow_rotated_crs_drop_2126.py | 140 ++++++++++++++++++ 3 files changed, 176 insertions(+), 17 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index dd51bed83..bd78f044b 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -382,13 +382,15 @@ def open_geotiff(source: str | BinaryIO, *, because the rest of xrspatial assumes an axis-aligned grid. ``allow_rotated=True`` reads the pixel grid without the geospatial assumption: the result has integer pixel coords on - ``x`` / ``y`` and ``attrs['crs']`` is dropped. The original - rotated 6-tuple is preserved on - ``geo_info.transform.rotated_affine`` for callers that want it - (issue #2115). The contract is read-only -- ``to_geotiff`` does - not currently emit ``rotated_affine``, so a read-then-write + ``x`` / ``y`` and both ``attrs['crs']`` and ``attrs['crs_wkt']`` + are dropped. The CRS attrs are dropped together with the + transform because keeping them while the axis-aligned transform + is gone misleads downstream code that gates on + ``"crs" in da.attrs`` to mean the array is spatially usable + (issue #2126). The contract is read-only -- ``to_geotiff`` does + not currently emit rotated transforms, so a read-then-write round-trip writes an identity-affine output and silently drops - the rotation. + the rotation (issue #2115). Returns ------- diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index 683640087..e104c69b5 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -355,23 +355,40 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None # rather than the bare literal. attrs['_xrspatial_geotiff_contract'] = _ATTRS_CONTRACT_VERSION - if geo_info.crs_epsg is not None: - attrs['crs'] = geo_info.crs_epsg - if geo_info.crs_wkt is not None: - attrs['crs_wkt'] = geo_info.crs_wkt + src_t = geo_info.transform + has_georef = getattr(geo_info, 'has_georef', True) + # ``allow_rotated=True`` opt-in path (#2115): the parser returns a + # GeoTransform with ``rotated_affine`` set and ``has_georef=False``. + # The rotated 6-tuple cannot be expressed as an axis-aligned + # rasterio transform, so the writer cannot round-trip it via + # ``attrs['transform']``. Per the documented contract on + # ``open_geotiff(allow_rotated=True)``, CRS attrs are dropped on + # this path too -- otherwise downstream code that gates on + # ``"crs" in da.attrs`` treats the array as spatially meaningful + # while the actual mapping is gone (#2126). + rotated_optin = ( + src_t is not None + and getattr(src_t, 'rotated_affine', None) is not None + and not has_georef + ) + + if not rotated_optin: + if geo_info.crs_epsg is not None: + attrs['crs'] = geo_info.crs_epsg + if geo_info.crs_wkt is not None: + attrs['crs_wkt'] = geo_info.crs_wkt if geo_info.raster_type == RASTER_PIXEL_IS_POINT: attrs['raster_type'] = 'point' - src_t = geo_info.transform # Skip the transform attr for files where no GeoTIFF transform tags # (ModelTransformation, ModelPixelScale, or ModelTiepoint) are # present, signalled by ``has_georef=False``. GeoKeys / CRS metadata - # can still be present in that case. The default unit - # ``GeoTransform`` is a struct placeholder, not real georef -- - # emitting it leaks an identity transform into attrs and confuses - # downstream code that expects ``'transform' in attrs`` to mean - # "this raster has a georef transform" (#1710). - has_georef = getattr(geo_info, 'has_georef', True) + # can still be present in that case (and are kept for plain + # no-georef files; the rotated opt-in path drops them above). The + # default unit ``GeoTransform`` is a struct placeholder, not real + # georef -- emitting it leaks an identity transform into attrs and + # confuses downstream code that expects ``'transform' in attrs`` to + # mean "this raster has a georef transform" (#1710). if src_t is not None and has_georef: attrs['transform'] = _transform_tuple_from_pixel_geometry( src_t.origin_x, src_t.origin_y, diff --git a/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py b/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py new file mode 100644 index 000000000..d2b45bd1a --- /dev/null +++ b/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py @@ -0,0 +1,140 @@ +"""``allow_rotated=True`` drops CRS attrs together with the transform (#2126). + +Pre-fix, ``_populate_attrs_from_geo_info`` wrote ``crs`` / ``crs_wkt`` +before checking ``has_georef``. On the ``allow_rotated=True`` opt-in +path (issue #2115) the transform was dropped but CRS attrs survived, +which mismatched the public docstring and misled downstream code that +gated on ``"crs" in da.attrs`` to mean "spatially meaningful". + +The fix detects the rotated opt-in path (transform carries +``rotated_affine`` and ``has_georef`` is False) and skips the CRS +writes there. Plain no-georef files (no transform tags at all) still +keep their CRS attrs -- only the rotated path drops them. +""" +from __future__ import annotations + +import io +import struct + +import numpy as np +import pytest + +from xrspatial.geotiff import open_geotiff +from xrspatial.geotiff._attrs import _populate_attrs_from_geo_info +from xrspatial.geotiff._geotags import GeoInfo, GeoTransform + + +def _rotated_geo_info(*, with_crs: bool = True) -> GeoInfo: + """Build a GeoInfo that mimics the allow_rotated=True parser output.""" + t = GeoTransform( + origin_x=0.0, origin_y=0.0, + pixel_width=1.0, pixel_height=-1.0, + rotated_affine=(8.66, -5.0, 100.0, 5.0, 8.66, 200.0), + ) + return GeoInfo( + transform=t, + has_georef=False, + crs_epsg=4326 if with_crs else None, + crs_wkt='GEOGCS["WGS 84"]' if with_crs else None, + ) + + +def test_rotated_optin_drops_crs_epsg(): + gi = _rotated_geo_info() + attrs = {} + _populate_attrs_from_geo_info(attrs, gi) + assert 'crs' not in attrs + assert 'transform' not in attrs + + +def test_rotated_optin_drops_crs_wkt(): + gi = _rotated_geo_info() + attrs = {} + _populate_attrs_from_geo_info(attrs, gi) + assert 'crs_wkt' not in attrs + + +def test_plain_no_georef_keeps_crs(): + # Plain no-georef file: no transform tag at all, but the GeoKey + # directory carries a CRS. This case must still emit ``crs`` so + # callers can display / reproject the array even without a transform. + gi = GeoInfo(transform=GeoTransform(), has_georef=False, crs_epsg=4326) + attrs = {} + _populate_attrs_from_geo_info(attrs, gi) + assert attrs.get('crs') == 4326 + assert 'transform' not in attrs + + +def test_axis_aligned_georef_keeps_crs_and_transform(): + gi = GeoInfo( + transform=GeoTransform( + origin_x=100.0, origin_y=200.0, + pixel_width=10.0, pixel_height=-10.0, + ), + has_georef=True, + crs_epsg=4326, + ) + attrs = {} + _populate_attrs_from_geo_info(attrs, gi) + assert attrs.get('crs') == 4326 + assert attrs.get('transform') == (10.0, 0.0, 100.0, 0.0, -10.0, 200.0) + + +# --------------------------------------------------------------------------- +# End-to-end via open_geotiff: write a rotated GeoTIFF with embedded CRS, +# read it with allow_rotated=True, confirm the attrs are clean. +# --------------------------------------------------------------------------- + + +def _write_rotated_tiff_with_geokeys(path, arr, *, epsg=4326): + """Build a synthetic rotated GeoTIFF with a GeoKey-encoded CRS.""" + tifffile = pytest.importorskip("tifffile") + # Rotated 4x4 model transformation tag (30-deg rotation, pixel size 10). + cos30 = 0.8660254037844387 + sin30 = 0.5 + m = ( + 10.0 * cos30, -10.0 * sin30, 0.0, 100.0, + 10.0 * sin30, 10.0 * cos30, 0.0, 200.0, + 0.0, 0.0, 1.0, 0.0, + 0.0, 0.0, 0.0, 1.0, + ) + # GeoKeyDirectory: header (4 shorts) + one key for GeographicTypeGeoKey + # (key id 2048, location 0 -> short value, count 1, value EPSG). + geo_key_directory = ( + 1, 1, 0, 1, # KeyDirectoryVersion=1, KeyRevision=1, MinorRevision=0, NumberOfKeys=1 + 2048, 0, 1, int(epsg), + ) + extratags = [ + # ModelTransformationTag (34264) -- DOUBLE, count=16. + (34264, 12, 16, m, False), + # GeoKeyDirectoryTag (34735) -- SHORT, count=8. + (34735, 3, 8, geo_key_directory, False), + ] + tifffile.imwrite( + path, arr, photometric='minisblack', + planarconfig='contig', extratags=extratags, + ) + + +def test_open_geotiff_rotated_with_crs_drops_crs(tmp_path): + arr = np.arange(20, dtype=' Date: Tue, 19 May 2026 07:56:31 -0700 Subject: [PATCH 2/4] geotiff: address review nits on PR 2128 (#2126) - Drop unused ``import io`` / ``import struct`` from ``test_allow_rotated_crs_drop_2126.py``. Follow-up filed as #2129 for surfacing ``attrs['rotated_affine']`` on the rotated opt-in path -- API surface expansion that warrants its own PR. --- xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py b/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py index d2b45bd1a..03734ce3d 100644 --- a/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py +++ b/xrspatial/geotiff/tests/test_allow_rotated_crs_drop_2126.py @@ -13,9 +13,6 @@ """ from __future__ import annotations -import io -import struct - import numpy as np import pytest From b0edaa5b6a51ca71610ef3b273cd66773fb247f4 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 08:19:59 -0700 Subject: [PATCH 3/4] tests: gate VRT int-source tests on tifffile (#2092, dup of #2140) CI failed on the merged PR 2128 because the new VRT tests imported tifffile unconditionally, but tifffile is not in the [tests] extras. PR 2140 fixes this on a separate branch; cherry-picking the same one-line change here so this PR's CI can land. When 2140 merges first the change becomes a no-op git resolution. --- xrspatial/geotiff/tests/test_masked_nodata_attr_2092.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xrspatial/geotiff/tests/test_masked_nodata_attr_2092.py b/xrspatial/geotiff/tests/test_masked_nodata_attr_2092.py index 70b5e5d05..ad35eab84 100644 --- a/xrspatial/geotiff/tests/test_masked_nodata_attr_2092.py +++ b/xrspatial/geotiff/tests/test_masked_nodata_attr_2092.py @@ -202,7 +202,7 @@ def _write_int_vrt(tmp_path, src_basename, vrt_basename, sentinel=30): in-repo VRT reader; without them the reader returns a zero-fill buffer instead of decoding the source. """ - import tifffile + tifffile = pytest.importorskip("tifffile") src = str(tmp_path / src_basename) tifffile.imwrite(src, np.array( [[10, 20, 30], [40, 50, 60]], dtype=np.int16, From 1996fb91126c4bac73421ff352fea5c503502270 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 11:41:31 -0700 Subject: [PATCH 4/4] ci: trigger RTD rebuild (prior build hit 902s timeout)