From 7c790a31f44aeb96c106d36122a1d5b830fe3f8f Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Fri, 15 May 2026 07:18:57 -0700 Subject: [PATCH] geotiff: write_geotiff_gpu kwarg order matches to_geotiff (#1922) to_geotiff and write_geotiff_gpu are documented parity siblings, but the two signatures put max_z_error and streaming_buffer_bytes in opposite order: to_geotiff: ..., bigtiff, gpu, streaming_buffer_bytes, max_z_error, photometric, ... write_geotiff_gpu: ..., bigtiff, max_z_error, streaming_buffer_bytes, photometric, ... Both kwargs are keyword-only so caller code does not break, but the drift surfaced in inspect.signature, IDE autocomplete, and Sphinx docs against the GPU writer's own "Accepted at the signature level for API parity with to_geotiff" docstring promise. Reorder write_geotiff_gpu to match to_geotiff. The 'gpu' auto-dispatch kwarg is the only one to_geotiff has that the GPU entry point does not, so the gap stays in place; everything else after 'bigtiff' lines up. Also reorder the docstring to keep doc order parallel. New regression test test_writer_kwarg_order_1922.py pins kwarg order parity (modulo 'gpu') and shared-default parity so future signature edits trip the test instead of silently re-introducing drift. Detected by deep-sweep-api-consistency on 2026-05-15. Refs #1922 --- .claude/sweep-api-consistency-state.csv | 2 +- xrspatial/geotiff/_writers/gpu.py | 12 +-- .../tests/test_writer_kwarg_order_1922.py | 76 +++++++++++++++++++ 3 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_writer_kwarg_order_1922.py diff --git a/.claude/sweep-api-consistency-state.csv b/.claude/sweep-api-consistency-state.csv index 6b5d4f921..85ee7521e 100644 --- a/.claude/sweep-api-consistency-state.csv +++ b/.claude/sweep-api-consistency-state.csv @@ -1,3 +1,3 @@ module,last_inspected,issue,severity_max,categories_found,notes -geotiff,2026-05-15,1845-followup,HIGH,5,"Sweep 2026-05-15 (deep-sweep-api-consistency-geotiff-2026-05-15). 1 HIGH Cat 5 finding fixed in this branch: to_geotiff was missing allow_internal_only_jpeg, the opt-in flag added to write_geotiff_gpu in #1845. to_geotiff(compression='jpeg', gpu=True, allow_internal_only_jpeg=True) could not reach the GPU writer's opt-in because to_geotiff rejected jpeg up front. Fix mirrors the GPU writer: accept the kwarg with default False, gate the up-front jpeg rejection on it, emit GeoTIFFFallbackWarning on opt-in, forward to write_geotiff_gpu. Regression test in test_to_geotiff_allow_internal_only_jpeg_parity.py (6 tests). Prior findings (#1654 #1683 #1684 #1685 #1705 #1715 #1754 #1775 #1810) all confirmed fixed. Cross-sibling return-type drift (Cat 2): write_vrt returns str while to_geotiff and write_geotiff_gpu return None -- still deferred (LOW, callers do not substitute these writers). cuda-validated." +geotiff,2026-05-15,1922,MEDIUM,1,"Sweep 2026-05-15 (deep-sweep-api-consistency-geotiff-2026-05-15-1778854324). 1 MEDIUM Cat 1 finding fixed in this branch: write_geotiff_gpu and to_geotiff disagreed on order of max_z_error / streaming_buffer_bytes kwargs. Both kwargs are keyword-only so no functional break; drift surfaced in inspect.signature, IDE autocomplete, and Sphinx docs against the writers' explicit-parity promise. Fix reorders write_geotiff_gpu to match to_geotiff (streaming_buffer_bytes before max_z_error) and updates the docstring; gpu is the only kwarg to_geotiff has that write_geotiff_gpu does not, so the gap stays. Regression test in test_writer_kwarg_order_1922.py pins kwarg order parity and default-value parity. Prior findings (#1654 #1683 #1684 #1685 #1705 #1715 #1754 #1775 #1810 #1845-followup) all confirmed fixed. Cross-sibling return-type drift (Cat 2): write_vrt returns str while to_geotiff and write_geotiff_gpu return None -- still deferred (LOW, callers do not substitute these writers). Cross-cutting cross-module drift (chunk_size in reproject vs chunks in geotiff; target_crs vs crs) documented but not filed per sweep template (cross-cutting). cuda-validated." reproject,2026-05-10,1570,HIGH,2;5,"Filed cross-module attrs['vertical_crs'] type collision (string vs EPSG int) vs xrspatial.geotiff. Fixed in PR (TBD): reproject now writes EPSG int and preserves friendly token under vertical_datum. MEDIUM kwarg-order drift (transform_precision vs chunk_size) and missing type hints vs geotiff documented but not fixed (cosmetic, kwarg-only)." diff --git a/xrspatial/geotiff/_writers/gpu.py b/xrspatial/geotiff/_writers/gpu.py index 155253fcf..5617c1d24 100644 --- a/xrspatial/geotiff/_writers/gpu.py +++ b/xrspatial/geotiff/_writers/gpu.py @@ -43,8 +43,8 @@ def write_geotiff_gpu(data: xr.DataArray | cupy.ndarray | np.ndarray, overview_levels: list[int] | None = None, overview_resampling: str = 'mean', bigtiff: bool | None = None, - max_z_error: float = 0.0, streaming_buffer_bytes: int = 256 * 1024 * 1024, + max_z_error: float = 0.0, photometric: str | int = 'auto', allow_internal_only_jpeg: bool = False) -> None: """Write a CuPy-backed DataArray as a GeoTIFF with GPU compression. @@ -146,17 +146,17 @@ def write_geotiff_gpu(data: xr.DataArray | cupy.ndarray | np.ndarray, bigtiff : bool or None Force BigTIFF (64-bit offsets). None auto-promotes when the estimated file size would exceed the classic-TIFF 4 GB limit. - max_z_error : float - Per-pixel error budget for LERC compression. The GPU writer - does not implement LERC (nvCOMP has no LERC backend), so any - non-zero value raises ``ValueError``. Accepted at the signature - level for API parity with ``to_geotiff``. streaming_buffer_bytes : int Accepted for API parity with ``to_geotiff``. The GPU writer materialises the entire array on device and has no streaming concept, so this kwarg is a no-op. Default matches ``to_geotiff`` (256 MB) so callers passing the same kwargs to either entry point see the same default and the same type. + max_z_error : float + Per-pixel error budget for LERC compression. The GPU writer + does not implement LERC (nvCOMP has no LERC backend), so any + non-zero value raises ``ValueError``. Accepted at the signature + level for API parity with ``to_geotiff``. photometric : str or int Photometric interpretation for the TIFF Photometric tag (262). See :func:`to_geotiff` for the full set of accepted values; the diff --git a/xrspatial/geotiff/tests/test_writer_kwarg_order_1922.py b/xrspatial/geotiff/tests/test_writer_kwarg_order_1922.py new file mode 100644 index 000000000..1da66d3dd --- /dev/null +++ b/xrspatial/geotiff/tests/test_writer_kwarg_order_1922.py @@ -0,0 +1,76 @@ +"""Regression test for #1922: write_geotiff_gpu kwarg order matches +to_geotiff (with the documented exception for ``gpu``). + +The two writers are advertised as parity siblings. The GPU writer's +own docstring says "Accepted at the signature level for API parity with +``to_geotiff``" for ``max_z_error`` and ``streaming_buffer_bytes``, but +the two kwargs were in opposite order across the two signatures: + + to_geotiff: ..., bigtiff, gpu, streaming_buffer_bytes, + max_z_error, photometric, ... + write_geotiff_gpu: ..., bigtiff, max_z_error, + streaming_buffer_bytes, photometric, ... + +Both are keyword-only so calling code did not break, but +``inspect.signature()``, IDE autocomplete, and Sphinx-rendered docs all +exposed the drift. Detected by deep-sweep-api-consistency on 2026-05-15. +""" +from __future__ import annotations + +import inspect + +from xrspatial.geotiff import to_geotiff, write_geotiff_gpu + + +def test_writer_kwarg_order_matches_to_geotiff(): + """``write_geotiff_gpu`` lists its kwargs in the same order as + ``to_geotiff``, modulo the ``gpu`` kwarg the GPU writer omits. + + Both signatures use keyword-only kwargs so positional callers are + unaffected. The order still matters for IDE autocomplete, generated + docs, and any caller that inspects ``inspect.signature``. + """ + eager_params = list(inspect.signature(to_geotiff).parameters) + gpu_params = list(inspect.signature(write_geotiff_gpu).parameters) + + # to_geotiff has ``gpu`` (auto-dispatch flag); write_geotiff_gpu does + # not. Drop it from the comparison instead of asserting on the + # missing kwarg directly, so unrelated future additions to either + # signature still surface here. + assert 'gpu' in eager_params + assert 'gpu' not in gpu_params + eager_params_no_gpu = [p for p in eager_params if p != 'gpu'] + + assert gpu_params == eager_params_no_gpu, ( + "write_geotiff_gpu and to_geotiff kwarg order diverged.\n" + f" to_geotiff (with 'gpu' removed): {eager_params_no_gpu}\n" + f" write_geotiff_gpu: {gpu_params}\n" + "Reorder write_geotiff_gpu to match to_geotiff (see #1922)." + ) + + +def test_writer_kwarg_defaults_match_to_geotiff(): + """The kwargs both writers share also have identical defaults. + + A surprise-free dispatch ``to_geotiff(..., gpu=True)`` requires + ``write_geotiff_gpu`` to default the same way for every kwarg the + auto-dispatch entry point forwards (issue #1916 added + ``allow_internal_only_jpeg`` to satisfy that contract; this test + pins the broader parity). + """ + eager_sig = inspect.signature(to_geotiff) + gpu_sig = inspect.signature(write_geotiff_gpu) + + shared = set(eager_sig.parameters) & set(gpu_sig.parameters) + # ``data`` and ``path`` are required positionals with no default; + # comparing inspect.Parameter.empty against itself is fine. + mismatches = [] + for name in sorted(shared): + ed = eager_sig.parameters[name].default + gd = gpu_sig.parameters[name].default + if ed != gd: + mismatches.append((name, ed, gd)) + assert not mismatches, ( + "write_geotiff_gpu and to_geotiff disagree on defaults: " + f"{mismatches}" + )