Raise RotatedTransformError for rotated GeoTIFFs#2272
Conversation
The geotag parser used a bare `NotImplementedError` for rotated `ModelTransformationTag` inputs, while the VRT path raised the typed `RotatedTransformError`. A caller that wrote `except RotatedTransformError` only caught the VRT case. Swap the raise in `_geotags._extract_transform_and_georef` for the typed error. The message and the `allow_rotated=True` opt-out stay intact. `RotatedTransformError` subclasses `ValueError`, so callers catching `ValueError` are unaffected. Update the stale comment in `_attrs.py:813` whose premise no longer holds, the public docstrings in `__init__.py`, and the existing tests that asserted `NotImplementedError`. Add `test_rotated_typed_error_2267.py` to pin the GeoTIFF/VRT parity contract.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Raise RotatedTransformError for rotated GeoTIFFs (#2272)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
Add a
CHANGELOG.mdentry under### Unreleased. The repo lists
one bullet per behaviour change with the PR number, and the original
RotatedTransformErrorintroduction already has a #1987-era entry
there. This PR extends that typed error to the GeoTIFF entry point,
so a sibling bullet referencing(#2267)keeps the breaking-but-
intentional behaviour change visible to anyone scanning the
changelog before upgrading. (CHANGELOG.md, top of### Unreleased) -
Update the stale historical comment at
_backends/dask.py:206.
It still says rotated remote GeoTIFFs "raisedNotImplementedError
from_parse_cog_http_meta". Historically accurate for pre-#2130,
but anyone grepping for the typed-error contract will land on it
and be briefly confused. Either retype toRotatedTransformError
while keeping the historical framing, or add a(now RotatedTransformError after #2267)parenthetical. Cleanup only.
Nits (optional improvements)
-
test_extract_transform_rotated_does_not_raise_notimplemented
atxrspatial/geotiff/tests/test_rotated_typed_error_2267.py:137-153
is functionally identical to
test_extract_transform_rotated_raises_typed_errorjust above it.
RotatedTransformErrordoes not subclassNotImplementedError(it
inherits fromValueError), so the innerexcept NotImplementedError:
clause is unreachable and the test reduces to
pytest.raises(RotatedTransformError). Either drop it or rewrite
it toassert not issubclass(RotatedTransformError, NotImplementedError),
which actually pins the type-hierarchy contract the docstring
claims to guard.
What looks good
- The exception swap at
_geotags.py:684keeps the original message
text: tag name, coordinate term values,allow_rotated=Truehint.
No diagnostic regression. - Tests hit both the unit surface (
_extract_transformon a
synthetic IFD) and the end-to-end surface (open_geotiffon a real
on-disk rotated TIFF), and the existing #2115 / #2130 / #2136 /
#2132 fixtures cover the dask and HTTP variants. - The stale comment in
_attrs.py:813-820is rewritten, not just
patched, so it now describes the symmetric contract instead of
contradicting it. test_rotated_error_is_geotiff_ambiguous_subclassexplicitly pins
theGeoTIFFAmbiguousMetadataErrorandValueErrorinvariants,
so the legacyexcept ValueErrorpath is locked down.
Checklist
- Algorithm matches reference/paper - N/A, no algorithm change
- All implemented backends produce consistent results - GeoTIFF
and VRT now share the typed error; dask and HTTP paths go through
the same parser - NaN handling is correct - N/A
- Edge cases are covered by tests - rotated + opt-in, rotated +
default, axis-aligned passthrough; rotated + dask chunked; rotated- HTTP
- Dask chunk boundaries handled correctly - N/A (error path)
- No premature materialization or unnecessary copies - N/A
- Benchmark exists or is not needed - not needed (error-path
change) - README feature matrix updated (if applicable) - not applicable
- Docstrings present and accurate -
__init__.pydocstrings
updated
…2267) - Add a CHANGELOG entry under Unreleased documenting the GeoTIFF parity extension of RotatedTransformError. - Update the stale historical comment at _backends/dask.py:206 with a parenthetical naming the current typed error. - Replace test_extract_transform_rotated_does_not_raise_notimplemented with test_rotated_error_not_a_notimplemented_subclass, which pins the type-hierarchy invariant directly (the old form's inner except NotImplementedError clause was unreachable because RotatedTransformError inherits from ValueError).
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (after f02b607)
Disposition of the original review findings:
Suggestions
- CHANGELOG entry -- added under
### Unreleasednext to the
existing #1987 entry that introducedRotatedTransformError,
describing the GeoTIFF parity extension and the breaking-but-
intentional half (callers catchingNotImplementedError
specifically must switch toRotatedTransformErroror
ValueError). - Stale comment at
_backends/dask.py:206-- updated with a
(now RotatedTransformError after #2267)parenthetical, preserving
the pre-#2130 historical framing.
Nits
- Redundant regression test -- rewrote as
test_rotated_error_not_a_notimplemented_subclass, which uses
issubclassto pin the type-hierarchy invariant directly. Also
pins the inverse (NotImplementedErroris not a
GeoTIFFAmbiguousMetadataError) so accidental hierarchy drift in
either direction fails loudly.
All 6 tests in test_rotated_typed_error_2267.py still pass; the
rotated-related fixtures across test_allow_rotated_geotiff_2115,
test_http_dask_allow_rotated_2130, test_georef_status_2136,
test_backend_parity_matrix, test_geotags, and
test_ambiguous_metadata_hooks_1987 are unchanged and green.
Closes #2267.
Summary
NotImplementedErrorin_geotags._extract_transform_and_georefwith the typedRotatedTransformError, so rotated GeoTIFFs and rotated VRTs shareone exception contract.
_attrs.py:813and the__init__.pydocstrings that referenced the old error type.
NotImplementedErroron the rotated
ModelTransformationTagpath.test_rotated_typed_error_2267.pyto pin the parity: parserraises
RotatedTransformError, subclassesGeoTIFFAmbiguousMetadataErrorandValueError, message stillnames the tag, and
allow_rotated=Truestill reads the pixel grid.Backend coverage
Read-path only (numpy + dask). No GPU code touched. Writers are not
affected; the existing
drop_rotationplumbing into_geotiffstaysas-is.
Test plan
pytest xrspatial/geotiff/tests/test_rotated_typed_error_2267.pypytest xrspatial/geotiff/tests/test_allow_rotated_geotiff_2115.py xrspatial/geotiff/tests/test_geotags.py xrspatial/geotiff/tests/test_georef_status_2136.py xrspatial/geotiff/tests/test_remaining_fail_closed_1987.py xrspatial/geotiff/tests/test_ambiguous_metadata_hooks_1987.py xrspatial/geotiff/tests/test_http_dask_allow_rotated_2130.py xrspatial/geotiff/tests/test_backend_parity_matrix.py- all greenpytest xrspatial/geotiff/tests/- only pre-existing failure(
test_lowlevel_write_pushdown_2138::test_write_vs_to_geotiff_byte_parity_uint8[lz4],unrelated lz4 experimental-codec gate) remains.
Compatibility note
RotatedTransformErroris aValueErrorsubclass, so anything thatcaught
ValueErrorkeeps catching the rotated case. Code that wroteexcept NotImplementedErrorspecifically for the rotated branch willnot catch it any more, which is the intentional half of the fix: that
contract was the bug.