Skip to content

geotiff: refuse to silently drop attrs['rotated_affine'] on write#2221

Merged
brendancol merged 3 commits into
mainfrom
issue-2216
May 21, 2026
Merged

geotiff: refuse to silently drop attrs['rotated_affine'] on write#2221
brendancol merged 3 commits into
mainfrom
issue-2216

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2216.

Summary

  • to_geotiff / write_geotiff_gpu / _write_vrt_tiled now refuse a DataArray that carries attrs['rotated_affine'] and raise ValueError naming the attr; the silent identity-affine round-trip the reader's docstring warned about at _geotags.py:635 is no longer reachable.
  • A new drop_rotation: bool = False kwarg on both writers lets the caller accept the loss explicitly. The docstring spells out the silent-loss consequence so callers learn the cost in one read.
  • Shared _validate_no_rotated_affine helper in _validation.py so all three writer entry points share one gate.

Backend coverage

  • numpy: covered (eager path).
  • dask + numpy: covered (streaming write path, exercised in test_round_trip_dask_rotated_tiff_requires_opt_in).
  • cupy: covered (write_geotiff_gpu runs the same gate; the GPU dispatch in to_geotiff forwards drop_rotation).
  • dask + cupy: covered (the eager-path gate fires before any backend selection).
  • VRT: covered (gate runs in both the to_geotiff(.vrt) dispatch and a direct _write_vrt_tiled call).

Test plan

  • pytest xrspatial/geotiff/tests/test_to_geotiff_drop_rotation_2216.py (10 passed).
  • pytest xrspatial/geotiff/tests/ (4649 passed; one pre-existing lz4 byte-parity failure on main unrelated to this change).
  • Reject without opt-in raises with a message naming rotated_affine and drop_rotation=True.
  • drop_rotation=True succeeds, pixels round-trip unchanged, the read-back attr is absent.
  • Plain rasters with no rotated_affine attr behave exactly as before.
  • attrs['rotated_affine'] = None does not trigger the gate.
  • Read-then-write-then-read on a synthetic rotated TIFF (both eager and chunked).

)

The reader exposes the rotated 6-tuple on attrs['rotated_affine'] when
called with allow_rotated=True (issue #2129), but to_geotiff has no
ModelTransformationTag emit path (tracked in #2115). Without an explicit
gate the writer accepted such inputs and produced an identity-affine
output, losing the rotation without any signal to the caller.

Add a shared _validate_no_rotated_affine helper in _validation.py and
call it from to_geotiff, write_geotiff_gpu, and _write_vrt_tiled. The
default refuses the write with a ValueError that names the attr and
points at the opt-in; a new drop_rotation=True kwarg lets the caller
accept the loss explicitly. Docstrings on to_geotiff, write_geotiff_gpu,
and the open_geotiff allow_rotated entry document the new gate.

Tests in test_to_geotiff_drop_rotation_2216.py cover the rejection
path, the opt-in path, the non-rotated baseline, a None-valued attr,
and read-then-write-then-read round-trips on a synthetic rotated TIFF
(eager and dask). VRT path parity is pinned via the to_geotiff(.vrt)
dispatch and a direct _write_vrt_tiled call.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 21, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: geotiff: refuse to silently drop attrs['rotated_affine'] on write

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/geotiff/_writers/eager.py:1003 -- _write_vrt_tiled runs the gate with entry_point="to_geotiff" even when called directly. The function is underscore-prefixed, so the documented surface always reaches it via to_geotiff; on a direct call the message still says to_geotiff: refusing to write.... Cosmetic only.
  • xrspatial/geotiff/_validation.py:610 -- attrs.get('rotated_affine') is None skips both missing-key and explicit-None, which matches what test_rotated_affine_attr_2129.py already documents. An empty tuple would fall through to the rejection branch, which is conservative but defensible; a one-line comment in the helper would save the next reader from second-guessing the choice.

What looks good

  • One shared _validate_no_rotated_affine helper keeps the three writer entry points in lockstep, same pattern the file already uses for _validate_nodata_arg.
  • The gate fires before cupy is imported in write_geotiff_gpu (line 368 vs line 435), so the rejection works on hosts with no GPU stack installed.
  • Validator only touches attrs, never .data -- no dask compute is triggered for a lazy DataArray.
  • The error message names the offending attr and the opt-in kwarg in one sentence so callers learn the flag without re-reading the docstring.
  • Docstrings line up across read and write sides: open_geotiff(allow_rotated=...), to_geotiff(drop_rotation=...), write_geotiff_gpu(drop_rotation=...), and the _extract_transform reader-side note all point at the same #2216 / #2115 split.
  • Tests cover the four required cases plus the dask read path, the VRT dispatch, a direct _write_vrt_tiled call, and an attrs['rotated_affine'] = None no-op.
  • Test filenames follow the project's tmp_2216_* convention.

Checklist

  • Algorithm matches reference (N/A -- input validation gate)
  • All implemented backends produce consistent results
  • NaN handling is correct (N/A -- does not touch pixel data)
  • Edge cases are covered by tests
  • Dask chunk boundaries handled correctly (gate runs before chunked compute)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed
  • README feature matrix updated (N/A -- no new function)
  • Docstrings present and accurate

- ``_write_vrt_tiled`` now passes ``entry_point="_write_vrt_tiled"``
  to the rotated-affine validator so a direct caller of the private
  helper sees the actual entry point in the error message. The public
  ``to_geotiff`` dispatch already runs its own gate upstream, so the
  helper-name surface is only visible to direct callers.
- Add a one-line comment in ``_validate_no_rotated_affine`` explaining
  the ``is None`` semantics: missing key and explicit None both skip,
  and any other value (including an empty tuple) falls through to the
  rejection branch so a malformed marker fails closed.

Test updated to assert the helper name surfaces in the direct-call
rejection message.

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review (follow-up): geotiff: refuse to silently drop attrs['rotated_affine'] on write

Re-review after 0864a41 addressed the two nits from the previous pass.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

None.

What looks good

  • _write_vrt_tiled now reports its own name in the error message; a direct caller of the private helper learns which gate fired without having to read the wrapper.
  • _validate_no_rotated_affine carries a comment explaining the is None semantics, so a future reader knows that an empty tuple or list still trips the gate rather than slipping through.
  • The new test assertion (pytest.raises(..., match="_write_vrt_tiled")) pins the helper-name surface so a future entry_point change cannot regress the error message without flagging it in this test file.

Checklist

  • All previous review nits addressed
  • Test coverage extended to cover the helper-name surface
  • No new findings on the follow-up commit
  • Backend parity unchanged from previous pass

@brendancol brendancol merged commit f321105 into main May 21, 2026
5 checks passed
@brendancol brendancol deleted the issue-2216 branch May 27, 2026 14:49
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.

to_geotiff silently drops rotated_affine instead of failing closed

1 participant