geotiff: refuse georeferenced TIFFs with orientation 5-8 (#1765)#1771
Conversation
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.
There was a problem hiding this comment.
Pull request overview
This PR updates the GeoTIFF reader’s handling of TIFF Orientation values 5–8 for georeferenced datasets to avoid returning incorrect geographic coordinates: instead of warning and continuing, the CPU reader now raises NotImplementedError when CRS information is present.
Changes:
- In the CPU GeoTIFF read path, raise
NotImplementedErrorfor Orientation 5–8 whencrs_epsg/crs_wktis set. - Update orientation tests to assert the new failure mode for CRS-tagged files and to preserve the existing transpose behavior for non-georeferenced files.
- Add a regression guard ensuring Orientation=1 georeferenced files still read normally.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xrspatial/geotiff/_reader.py |
Replaces the prior warning behavior with a hard failure for georeferenced Orientation 5–8 cases (CRS present). |
xrspatial/geotiff/tests/test_orientation.py |
Updates/extends tests to validate the new exception behavior and guard non-georeferenced behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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, |
There was a problem hiding this comment.
Good catch, fixed. The original raise was gated on crs_epsg/crs_wkt being set, but the surrounding branch is reachable whenever has_georef=True (ModelTransformation, ModelPixelScale, or ModelTiepoint present). 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. That's exactly the contract #1765 was supposed to close.
The CPU helper now raises NotImplementedError for any has_georef=True orientation 5-8 case. The non-georef branch is unchanged (the pixel-size swap was dead code under has_georef=False anyway, since the has_georef=False branch above already runs pass). Added test_orientation_5_to_8_transform_only_raises to lock that in.
| @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)) |
There was a problem hiding this comment.
Right, GPU was still going through _apply_orientation_geo_info, which warned and returned the same wrong coords #1765 was meant to close. Updated the GPU helper to raise NotImplementedError for has_georef=True orientation 5-8 with the same message as the CPU path. Added three new GPU tests: a raise-on-georef parametrize, a raise-on-transform-only parametrize, and a regression guard that no-georef files still swap axes. All 40 tests in test_orientation_gpu.py pass.
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.
Summary
crs_epsg/crs_wktset now raiseNotImplementedErrorinstead of returning silently wrong coords behind a warning.Fixes #1765.
Test plan
pytest xrspatial/geotiff/tests/test_orientation.py -x -qpytest xrspatial/geotiff/tests/test_http_orientation_1717.py xrspatial/geotiff/tests/test_orientation_gpu.py -x -q