From 5864d17e316273f10fe33064106af782433a9783 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Wed, 13 May 2026 05:16:57 -0700 Subject: [PATCH 1/2] geotiff: refuse georeferenced TIFFs with orientation 5-8 (#1765) Orientations 5-8 need a per-orientation origin shift plus a rotation that the axis-aligned GeoTransform cannot represent. The reader used to swap pixel_width/pixel_height and warn, leaving the user with silently wrong x/y coords. Raise NotImplementedError on georef'd files instead. Non-georef files keep the axis swap. --- xrspatial/geotiff/_reader.py | 23 +++--- xrspatial/geotiff/tests/test_orientation.py | 83 +++++++++++++++------ 2 files changed, 72 insertions(+), 34 deletions(-) diff --git a/xrspatial/geotiff/_reader.py b/xrspatial/geotiff/_reader.py index b97d63213..4563a92c3 100644 --- a/xrspatial/geotiff/_reader.py +++ b/xrspatial/geotiff/_reader.py @@ -2258,22 +2258,25 @@ def _apply_orientation_with_geo( pixel_height=new_px_h, ) elif orientation in (5, 6, 7, 8): + if (geo_info.crs_epsg is not None + or geo_info.crs_wkt is not None): + raise NotImplementedError( + f"TIFF Orientation {orientation} on a georeferenced file " + f"requires a per-orientation origin shift plus a rotation " + f"that the axis-aligned GeoTransform used here cannot " + f"represent, so the returned x/y coords would be wrong. " + f"Reproject the file with another tool (e.g. GDAL) or " + f"strip the Orientation tag before reading. See issue " + f"#1765." + ) + # Non-georeferenced file: swap the pixel sizes to match the + # transposed array shape. No geographic claim to violate. geo_info.transform = GeoTransform( origin_x=t.origin_x, origin_y=t.origin_y, pixel_width=t.pixel_height, pixel_height=t.pixel_width, ) - if (geo_info.crs_epsg is not None - or geo_info.crs_wkt is not None): - import warnings - warnings.warn( - f"Orientation {orientation} swaps spatial axes on " - f"a georeferenced file; the returned coords are " - f"shape-correct but the geographic transform may " - f"need manual adjustment.", - stacklevel=2, - ) return arr, geo_info diff --git a/xrspatial/geotiff/tests/test_orientation.py b/xrspatial/geotiff/tests/test_orientation.py index 32c8773d1..9816c1888 100644 --- a/xrspatial/geotiff/tests/test_orientation.py +++ b/xrspatial/geotiff/tests/test_orientation.py @@ -193,47 +193,82 @@ def test_orientation_round_trip_does_not_double_apply(tmp_path): @pytest.mark.parametrize("orientation", [5, 6, 7, 8]) -def test_orientation_5_to_8_warn_on_georef(tmp_path, orientation): - """Axis-swap orientations on georef'd files emit a warning. - - The transform swap is shape-correct but geometrically approximate - for orientations 6/7/8; the user should be told. +def test_orientation_5_to_8_raise_on_georef(tmp_path, orientation): + """Axis-swap orientations on georef'd files raise NotImplementedError. + + Orientations 5-8 require a per-orientation origin shift plus a + rotation that the axis-aligned GeoTransform cannot represent. + The reader used to swap pixel_width/pixel_height and warn; that + produced silently wrong coords on georef'd files (issue #1765). + The reader now refuses the file instead. """ - import warnings as _warnings - arr = np.arange(24, dtype=np.uint8).reshape(4, 6) - path = tmp_path / f"orient_georef_{orientation}.tif" - # tifffile lets us tag a CRS via the resolution + metadata; the - # simplest path that triggers our georef branch is to embed a - # ModelPixelScale + GeoKeyDirectory pair pointing at EPSG:4326. + path = tmp_path / f"orient_georef_raise_1765_{orientation}.tif" + # ModelPixelScale + GeoKeyDirectory pair pointing at EPSG:4326 + # makes the reader treat this as a georeferenced file. tifffile.imwrite( str(path), arr, extratags=[ (274, 'H', 1, orientation, True), - # ModelPixelScaleTag (33550): scale_x, scale_y, scale_z (33550, 'd', 3, (1.0, 1.0, 0.0), True), - # ModelTiepointTag (33922): I, J, K, X, Y, Z (33922, 'd', 6, (0.0, 0.0, 0.0, 0.0, 0.0, 0.0), True), - # GeoKeyDirectoryTag (34735): version 1.1.0, 1 key, - # GTModelType=2 (Geographic), GeographicType=4326 (34735, 'H', 12, ( 1, 1, 0, 2, - 1024, 0, 1, 2, # GTModelTypeGeoKey = 2 - 2048, 0, 1, 4326, # GeographicTypeGeoKey = 4326 + 1024, 0, 1, 2, + 2048, 0, 1, 4326, ), True), ], ) - with _warnings.catch_warnings(record=True) as caught: - _warnings.simplefilter('always') - da = open_geotiff(str(path)) + with pytest.raises(NotImplementedError, match=str(orientation)): + open_geotiff(str(path)) - assert da is not None - msgs = [str(w.message) for w in caught if 'Orientation' in str(w.message)] - assert msgs, ( - f"orientation={orientation}: no warning emitted on georef file" + +@pytest.mark.parametrize("orientation", [5, 6, 7, 8]) +def test_orientation_5_to_8_no_geo_still_swaps(tmp_path, orientation): + """Without georef, orientations 5-8 still do the axis swap. + + No geographic claim to violate, so the existing transpose path is + preserved (regression guard for the #1765 fix not over-reaching). + """ + arr = np.arange(24, dtype=np.uint8).reshape(4, 6) + path = tmp_path / f"orient_no_geo_1765_{orientation}.tif" + tifffile.imwrite( + str(path), arr, + extratags=[(274, 'H', 1, orientation, True)], ) + da = open_geotiff(str(path)) + expected = _expected_for_orientation(arr, orientation) + assert da.values.shape == expected.shape + np.testing.assert_array_equal(da.values, expected) + + +def test_orientation_1_georef_unchanged_1765(tmp_path): + """orientation=1 on a georef'd file still reads normally. + + Regression guard: the #1765 raise must be scoped to 5-8, not fire + on any georeferenced file. + """ + arr = np.arange(24, dtype=np.uint8).reshape(4, 6) + path = tmp_path / "orient_georef_1_1765.tif" + tifffile.imwrite( + str(path), arr, + extratags=[ + (274, 'H', 1, 1, True), + (33550, 'd', 3, (1.0, 1.0, 0.0), True), + (33922, 'd', 6, (0.0, 0.0, 0.0, 0.0, 0.0, 0.0), True), + (34735, 'H', 12, ( + 1, 1, 0, 2, + 1024, 0, 1, 2, + 2048, 0, 1, 4326, + ), True), + ], + ) + + da = open_geotiff(str(path)) + np.testing.assert_array_equal(da.values, arr) + # --------------------------------------------------------------------------- # Geographic coordinate updates for mirror-flip orientations (issue #1537) From 1f5583cbb2c50ee24980b2b4e5198c4d7782416a Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Wed, 13 May 2026 06:04:59 -0700 Subject: [PATCH 2/2] geotiff: gate orientation 5-8 raise on has_georef, mirror on GPU path The original raise in `_apply_orientation_with_geo` was gated on `crs_epsg`/`crs_wkt` being set, but the surrounding branch is reachable whenever `has_georef=True`. A file carrying ModelPixelScale + ModelTiepoint without a GeoKeyDirectory would have `has_georef=True` and `crs_epsg=None`, so the old code fell through to the pixel-size swap and returned silently wrong coords. Gate on `has_georef` so the raise covers any transform-tagged file. The GPU reader's `_apply_orientation_geo_info` had the same bug: it warned and proceeded for orientation 5-8 on georef'd files, returning the same wrong coords #1765 was meant to close. Raise `NotImplementedError` with the same message as the CPU path so the two backends agree. Tests: - test_orientation_5_to_8_transform_only_raises (CPU): asserts transform-tagged-but-CRS-less files now raise. - test_gpu_orientation_5_to_8_raise_on_georef (GPU): parity with the CPU raise on fully-georef'd files. - test_gpu_orientation_5_to_8_transform_only_raises (GPU): GPU raises on transform-tagged-but-CRS-less files too. - test_gpu_orientation_5_to_8_no_georef_still_swaps (GPU): regression guard that the raise doesn't over-reach to plain Orientation-tagged files with no geo tags. Addresses both Copilot review comments on PR #1771. --- xrspatial/geotiff/__init__.py | 27 ++++--- xrspatial/geotiff/_reader.py | 33 ++++---- xrspatial/geotiff/tests/test_orientation.py | 24 ++++++ .../geotiff/tests/test_orientation_gpu.py | 75 +++++++++++++++++++ 4 files changed, 132 insertions(+), 27 deletions(-) diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index a30d75b99..0600b5919 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -2480,21 +2480,30 @@ def _apply_orientation_geo_info(geo_info, orientation: int, pixel_height=new_px_h, ) elif orientation in (5, 6, 7, 8): + # Match the CPU reader's #1765 refusal: a pixel-size swap alone + # cannot express the per-orientation origin shift plus rotation + # these orientations require, so the x/y coords would be wrong. + # ``has_georef`` is True for any file carrying ModelTransformation, + # ModelPixelScale, or ModelTiepoint, with or without a CRS tag, so + # gate on that flag rather than CRS presence. + if getattr(geo_info, 'has_georef', False): + raise NotImplementedError( + f"TIFF Orientation {orientation} on a georeferenced file " + f"requires a per-orientation origin shift plus a rotation " + f"that the axis-aligned GeoTransform used here cannot " + f"represent, so the returned x/y coords would be wrong. " + f"Reproject the file with another tool (e.g. GDAL) or " + f"strip the Orientation tag before reading. See issue " + f"#1765." + ) + # Non-georeferenced file: swap pixel sizes to match the + # transposed array shape. No geographic claim to violate. geo_info.transform = GeoTransform( origin_x=t.origin_x, origin_y=t.origin_y, pixel_width=t.pixel_height, pixel_height=t.pixel_width, ) - if (geo_info.crs_epsg is not None - or geo_info.crs_wkt is not None): - warnings.warn( - f"Orientation {orientation} swaps spatial axes on " - f"a georeferenced file; the returned coords are " - f"shape-correct but the geographic transform may " - f"need manual adjustment.", - stacklevel=3, - ) return geo_info diff --git a/xrspatial/geotiff/_reader.py b/xrspatial/geotiff/_reader.py index 4563a92c3..beb151b49 100644 --- a/xrspatial/geotiff/_reader.py +++ b/xrspatial/geotiff/_reader.py @@ -2258,24 +2258,21 @@ def _apply_orientation_with_geo( pixel_height=new_px_h, ) elif orientation in (5, 6, 7, 8): - if (geo_info.crs_epsg is not None - or geo_info.crs_wkt is not None): - raise NotImplementedError( - f"TIFF Orientation {orientation} on a georeferenced file " - f"requires a per-orientation origin shift plus a rotation " - f"that the axis-aligned GeoTransform used here cannot " - f"represent, so the returned x/y coords would be wrong. " - f"Reproject the file with another tool (e.g. GDAL) or " - f"strip the Orientation tag before reading. See issue " - f"#1765." - ) - # Non-georeferenced file: swap the pixel sizes to match the - # transposed array shape. No geographic claim to violate. - geo_info.transform = GeoTransform( - origin_x=t.origin_x, - origin_y=t.origin_y, - pixel_width=t.pixel_height, - pixel_height=t.pixel_width, + # ``has_georef`` is True whenever ModelTransformation, + # ModelPixelScale, or ModelTiepoint is present, even without a + # CRS. The pixel-size swap below cannot express the + # per-orientation origin shift plus rotation these orientations + # require, so the x/y coords would be wrong whether or not a + # CRS tag accompanies the transform. Refuse the file in that + # case rather than warn and return silently wrong coords. + raise NotImplementedError( + f"TIFF Orientation {orientation} on a georeferenced file " + f"requires a per-orientation origin shift plus a rotation " + f"that the axis-aligned GeoTransform used here cannot " + f"represent, so the returned x/y coords would be wrong. " + f"Reproject the file with another tool (e.g. GDAL) or " + f"strip the Orientation tag before reading. See issue " + f"#1765." ) return arr, geo_info diff --git a/xrspatial/geotiff/tests/test_orientation.py b/xrspatial/geotiff/tests/test_orientation.py index 9816c1888..503a05def 100644 --- a/xrspatial/geotiff/tests/test_orientation.py +++ b/xrspatial/geotiff/tests/test_orientation.py @@ -224,6 +224,30 @@ def test_orientation_5_to_8_raise_on_georef(tmp_path, orientation): open_geotiff(str(path)) +@pytest.mark.parametrize("orientation", [5, 6, 7, 8]) +def test_orientation_5_to_8_transform_only_raises(tmp_path, orientation): + """``has_georef`` without CRS still triggers the raise. + + A TIFF carrying ModelPixelScale + ModelTiepoint but no + GeoKeyDirectory has ``has_georef=True`` and ``crs_epsg=None``. The + pixel-size swap alone misses the per-orientation origin shift, so + refusing is the honest contract regardless of CRS tagging. + """ + arr = np.arange(24, dtype=np.uint8).reshape(4, 6) + path = tmp_path / f"orient_transform_only_1765_{orientation}.tif" + tifffile.imwrite( + str(path), arr, + extratags=[ + (274, 'H', 1, orientation, True), + (33550, 'd', 3, (1.0, 1.0, 0.0), True), + (33922, 'd', 6, (0.0, 0.0, 0.0, 0.0, 0.0, 0.0), True), + ], + ) + + with pytest.raises(NotImplementedError, match=str(orientation)): + open_geotiff(str(path)) + + @pytest.mark.parametrize("orientation", [5, 6, 7, 8]) def test_orientation_5_to_8_no_geo_still_swaps(tmp_path, orientation): """Without georef, orientations 5-8 still do the axis swap. diff --git a/xrspatial/geotiff/tests/test_orientation_gpu.py b/xrspatial/geotiff/tests/test_orientation_gpu.py index 72ebe71b3..16f9f046a 100644 --- a/xrspatial/geotiff/tests/test_orientation_gpu.py +++ b/xrspatial/geotiff/tests/test_orientation_gpu.py @@ -180,3 +180,78 @@ def test_gpu_default_orientation_unchanged(tmp_path): da = read_geotiff_gpu(str(path)) np.testing.assert_array_equal(da.data.get(), arr) + + +@_gpu_only +@pytest.mark.parametrize("orientation", [5, 6, 7, 8]) +def test_gpu_orientation_5_to_8_raise_on_georef(tmp_path, orientation): + """GPU reader refuses georef'd files with axis-swap orientations. + + Mirrors the CPU behaviour added for issue #1765. ``read_geotiff_gpu`` + used to warn and return silently wrong x/y coords for these cases. + """ + from xrspatial.geotiff import read_geotiff_gpu + + arr = np.arange(256, dtype=np.uint8).reshape(16, 16) + path = tmp_path / f"gpu_orient_georef_raise_1765_{orientation}.tif" + _write_tiled( + path, arr, orientation, + extra=[ + (33550, 'd', 3, (1.0, 1.0, 0.0), True), + (33922, 'd', 6, (0.0, 0.0, 0.0, 100.0, 50.0, 0.0), True), + (34735, 'H', 12, ( + 1, 1, 0, 2, + 1024, 0, 1, 2, + 2048, 0, 1, 4326, + ), True), + ], + ) + + with pytest.raises(NotImplementedError, match=str(orientation)): + read_geotiff_gpu(str(path)) + + +@_gpu_only +@pytest.mark.parametrize("orientation", [5, 6, 7, 8]) +def test_gpu_orientation_5_to_8_transform_only_raises(tmp_path, orientation): + """``has_georef`` without CRS still triggers the raise on GPU. + + Files carrying ModelPixelScale + ModelTiepoint but no + GeoKeyDirectory have ``has_georef=True``/``crs_epsg=None``. The + pixel-size swap alone misses the per-orientation origin shift, so + refusing is the honest contract regardless of CRS tagging. + """ + from xrspatial.geotiff import read_geotiff_gpu + + arr = np.arange(256, dtype=np.uint8).reshape(16, 16) + path = tmp_path / f"gpu_orient_transform_only_1765_{orientation}.tif" + _write_tiled( + path, arr, orientation, + extra=[ + (33550, 'd', 3, (1.0, 1.0, 0.0), True), + (33922, 'd', 6, (0.0, 0.0, 0.0, 100.0, 50.0, 0.0), True), + ], + ) + + with pytest.raises(NotImplementedError, match=str(orientation)): + read_geotiff_gpu(str(path)) + + +@_gpu_only +@pytest.mark.parametrize("orientation", [5, 6, 7, 8]) +def test_gpu_orientation_5_to_8_no_georef_still_swaps(tmp_path, orientation): + """Without any geo tags, GPU orientation 5-8 still swaps axes. + + Regression guard for the #1765 GPU fix: refusing must be scoped to + georeferenced files, not every Orientation 5-8 read. + """ + from xrspatial.geotiff import read_geotiff_gpu + + arr = np.arange(256, dtype=np.uint8).reshape(16, 16) + path = tmp_path / f"gpu_orient_no_geo_1765_{orientation}.tif" + _write_tiled(path, arr, orientation) + + da = read_geotiff_gpu(str(path)) + expected = _expected_for_orientation(arr, orientation) + assert da.data.shape == expected.shape + np.testing.assert_array_equal(da.data.get(), expected)