Pin actionable failure modes for unsupported COG writer inputs (#2301)

[brendancol]

Summary

Pins typed, actionable exceptions for unsupported to_geotiff(..., cog=True) input combinations, per the matrix in #2301. Most rows already raised; the test file just locks the type and a substring of the message in. One row needed a writer-side hook.

Behavior matrix

Row	Already raised?	Writer change?
Experimental codec without allow_experimental_codecs=True	yes, ValueError naming codec + flag + tier	none
Internal-only JPEG without allow_internal_only_jpeg=True	yes, ValueError naming codec + flag	none
attrs['rotated_affine'] set without drop_rotation=True	yes, ValueError (#2216)	none
attrs['transform'] 6-tuple with rotation/shear	yes, ValueError (transform_from_attr, #1987 PR 3)	none
attrs['transform'] rasterio Affine with rotation/shear	no, silently dropped	new check in to_geotiff
BytesIO destination with cog=True	yes, ValueError	none
CuPy / GPU array with cog=True	currently succeeds, documented as Experimental tier	none (no semantic change per task scope)
Object dtype	yes, ValueError naming dtype	none
Conflicting attrs['crs'] vs attrs['crs_wkt']	yes, ConflictingCRSError (#1987 PR 6)	none

The one production change

A rasterio Affine iterates as a 9-element augmented matrix, so the 6-tuple branch in transform_from_attr returned None for it and the rotation/shear gate never fired. The writer then fell back to coord-derived or no-georef output and the rotation was lost on disk. to_geotiff now duck-types the Affine via .b / .d attrs and raises the same diagnostic the 6-tuple branch already produces, so the rejection message stays identical across both input shapes.

Test plan

• 18 new tests pass locally
• test_cog.py, test_allow_rotated_*.py, test_cog_writer_compliance.py still pass
• test_georef_resolver_parity_2211.py, test_remaining_fail_closed_1987.py, test_ambiguous_metadata_hooks_1987.py still pass

Closes #2301

[brendancol]

PR Review: Pin actionable failure modes for unsupported COG writer inputs (#2301)

Blockers

None.

Suggestions

• xrspatial/geotiff/_writers/eager.py:378-380 — the try: _b = float(...); _d = float(...) except (TypeError, ValueError): _b = _d = 0.0 swallow path silently turns a malformed attrs['transform'] (e.g. transform.b set to a string) into "no rotation, proceed with the write". That used to be the no-op fall-through in transform_from_attr, but here it bypasses every downstream gate that would have caught the bad value (the resolver and coords_to_transform). Consider re-raising as a ValueError naming the attr instead of zero-defaulting. Low risk in practice (real Affine objects always have float .b / .d) but the dead branch reads as "fail open" inside a fail-closed validator.

• xrspatial/geotiff/_writers/eager.py:360-389 — the duck-typed Affine detection fixes the public to_geotiff entry point, but write_geotiff_gpu is also a public-ish entry (used by the auto-dispatch path and exported on the package). If a caller invokes write_geotiff_gpu directly with a rotated Affine in attrs, the new gate doesn't fire there. Out of scope for this PR per the issue's "public entry" wording, but worth a follow-up note since both entry points were intended to "hit the same gate" per the existing _validate_no_rotated_affine comment two lines up.

Nits

• xrspatial/geotiff/tests/test_cog_invalid_input_errors_2286.py line ~188 (test_bytesio_destination_with_cog_raises) lacks the tmp_path fixture and writes nothing to disk; the test name suffix _2301 is also missing on the BytesIO buffer (other tests include it on file paths). Consistency nit only.

• xrspatial/geotiff/tests/test_cog_invalid_input_errors_2286.py line ~296 (test_crs_kwarg_overrides_attrs_silently) uses crs_wkt='GEOGCS["foo"]' to make the conflict check no-op. The comment says "unparseable; check no-ops" but the check actually short-circuits at if context.get('crs_kwarg') is not None: return before pyproj parsing. The comment overstates how the bypass works.

• xrspatial/geotiff/_writers/eager.py:382 — _ROT_TOL = 1e-12 is repeated verbatim from _coords.py:333. A shared constant would prevent the two gates drifting apart on a future tolerance tweak.

What looks good

• The new check is exactly as narrow as the issue asks: it touches one validator block inside to_geotiff, leaves every other path alone, and reuses the existing 6-tuple error message verbatim so existing pytest.raises(match=...) callers in the codebase still hit.
• Test coverage is thorough: 18 tests covering all seven matrix rows plus three sanity-guard rows that pin the happy paths (axis-aligned Affine writes, BytesIO without cog=True, CRS-kwarg override). The sanity guards protect against a future "tighten the rejection" change widening the bucket and breaking real callers.
• The CuPy + cog=True row is correctly handled as a no-op pin with a comment explaining the tier-promotion decision is out of scope. Many reviewers would have flipped this to a pytest.raises to "match the issue body", which would have changed semantics on a currently-succeeding path.
• The 6-tuple-rotated and Affine-rotated tests share the exact same 'rotation/shear' and 'axis-aligned' assertions, which locks the two code paths to one error message.
• Temporary file names include the issue suffix _2301 to avoid collisions with parallel test runs (matches the project convention).

Checklist

• Algorithm matches reference: N/A (validation hook, not numerical code)
• All implemented backends produce consistent results: covered (CuPy row pinned as currently-succeeds; rotated check applies pre-dispatch so GPU path is also gated when reached via to_geotiff)
• NaN handling: N/A
• Edge cases covered by tests: axis-aligned Affine sanity, drop_rotation opt-in, BytesIO without cog, crs-kwarg override
• Dask chunk boundaries handled correctly: N/A
• No premature materialization or unnecessary copies: N/A
• Benchmark exists or is not needed: not needed (validation-only)
• README feature matrix updated: N/A (no new functions)
• Docstrings present and accurate

[brendancol]

Follow-up review after 40f5cd1

All three suggestions / nits from the first pass are addressed in the follow-up commit:

• Suggestion 1 (fail-closed on malformed .b / .d): the zero-default branch is replaced with a typed ValueError naming the unconvertable values. A new test (test_affine_attr_with_unconvertable_b_d_raises) pins the behaviour.
• Suggestion 2 (write_geotiff_gpu direct-caller gap): deferred. The issue scopes #2301 to the public to_geotiff entry point, and the auto-dispatch already routes GPU writes through to_geotiff so the gate fires before GPU dispatch. Direct callers of write_geotiff_gpu are a follow-up, not a regression introduced here.
• Nit 1 (BytesIO test consistency): not changed. The test path lives in a BytesIO buffer with no on-disk component, so the _2301 filename suffix convention does not apply. Leaving as-is.
• Nit 2 (test comment overstates the bypass path): the docstring on test_crs_kwarg_overrides_attrs_silently now describes the actual short-circuit (kwarg presence in _check_write_conflicting_crs), not the WKT parseability.
• Nit 3 (shared _ROT_TOL): promoted to ROTATION_SHEAR_TOL at module level in _coords.py; both the 6-tuple branch in transform_from_attr and the Affine duck-type branch in to_geotiff import the same constant.

166 tests pass on the related test files (the new 19 plus the COG, rotated-transform, georef-resolver, fail-closed, and ambiguous-metadata suites). No remaining blockers from my side.