Skip to content

geotiff: document allow_rotated and shared kwargs on all read entry points (#2274)#2275

Merged
brendancol merged 3 commits into
mainfrom
deep-sweep-api-consistency-geotiff-2026-05-21-01
May 22, 2026
Merged

geotiff: document allow_rotated and shared kwargs on all read entry points (#2274)#2275
brendancol merged 3 commits into
mainfrom
deep-sweep-api-consistency-geotiff-2026-05-21-01

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2274.

Summary

  • Added Parameters-section entries on every read entry point for the two functional shared kwargs (allow_rotated, allow_unparseable_crs) and the five gated kwargs that previously sat in the signature without any docstring entry.
  • Short pointer entries reference open_geotiff / read_vrt / read_geotiff_gpu for the long form so the description lives in one place per kwarg.
  • New regression test parametrises over the four read entry points and asserts every signature kwarg has a numpy-style Parameters heading.

Scope

Documentation-only. No signature changes, no default changes, no dispatch logic changes.

Backends touched

Backend Doc change
numpy (open_geotiff) Added allow_unparseable_crs Parameters entry (was only in Tier prose)
dask (read_geotiff_dask) Added allow_rotated, allow_unparseable_crs, on_gpu_failure, missing_sources, max_cloud_bytes
cupy (read_geotiff_gpu) Added allow_rotated, allow_unparseable_crs, band_nodata, missing_sources, max_cloud_bytes
vrt (read_vrt) Added allow_rotated, allow_unparseable_crs, overview_level, on_gpu_failure, max_cloud_bytes

Test plan

  • pytest xrspatial/geotiff/tests/test_read_entry_points_doc_param_parity_2274.py — 16 cases pass
  • pytest xrspatial/geotiff/tests/test_open_geotiff_max_cloud_bytes_annot_2106.py — prior geotiff: open_geotiff(max_cloud_bytes=...) missing type annotation #2106 regression still passes
  • pytest xrspatial/geotiff/tests/ -k "annot or doc or signature or parity_dispatch or rotated" — 183 passed, 2 skipped
  • CUDA smoke test: read_geotiff_gpu(path, allow_rotated=True) returns CuPy-backed DataArray on a host with CUDA_AVAILABLE=true
  • CI green (will fix any breaks)

Origin

Found by the API consistency sweep for the geotiff module on 2026-05-21. Branch follows the sweep's fan-out naming convention.

…oints (#2274)

The four public read entry points (open_geotiff, read_geotiff_dask,
read_geotiff_gpu, read_vrt) accepted allow_rotated and
allow_unparseable_crs but only open_geotiff mentioned them (and
allow_unparseable_crs only in the Tier prose paragraph, not the
Parameters section). The three direct backends also accepted several
gated kwargs (missing_sources, band_nodata, on_gpu_failure,
max_cloud_bytes, overview_level on VRT) that raise ValueError on the
wrong backend for error symmetry; those had no Parameters entry on
the backends where they raise.

Add Parameters-section entries for every signature kwarg on every read
entry point. Functional kwargs get full descriptions; gated kwargs get
a short pointer at the owning backend so the long form lives in one
place.

Add test_read_entry_points_doc_param_parity_2274.py to pin the fix and
catch any future kwarg added without a matching Parameters entry on any
of the four readers.

Scope is documentation-only. No signature, default, or dispatch logic
changes. Verified via inspect.signature vs inspect.getdoc scan, plus
a CUDA smoke test of read_geotiff_gpu(allow_rotated=True).
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 21, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: docstring parity for geotiff read entry points

Blockers

None.

Suggestions

None.

Nits

  • xrspatial/geotiff/_backends/gpu.py, new band_nodata entry: "GPU reads do not go through the VRT pipeline" is a touch imprecise. The dispatcher rejects band_nodata for non-VRT sources; read_geotiff_gpu rejects all VRT sources up front. Current wording reads fine, just flagging.

What looks good

  • Diff is documentation-only as advertised. No signature, default, or dispatch logic changes.
  • The new regression test covers both directions (signature kwarg without a doc entry, and doc entry without a signature kwarg) and adds targeted assertions on the two shared kwargs the bug actually named.
  • Docstring claims about dispatch errors line up with the rules in _validation.py and the existing test_dispatch_validation_parity_2162.py / test_open_geotiff_on_gpu_failure_1615.py / test_open_geotiff_missing_sources_1810.py coverage.
  • Pattern matches the #2106 annotation-parity fix, which is a sensible model.

Checklist

  • Algorithm matches reference -- N/A, docs only
  • Backends consistent -- N/A, no behaviour change
  • NaN handling -- N/A
  • Edge cases tested -- 16 parametrised cases over the 4 readers
  • Dask chunk boundaries -- N/A
  • No premature materialization -- N/A
  • Benchmark needed -- N/A
  • README feature matrix -- N/A, no new function
  • Docstrings present and accurate -- the point of this PR

Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up: Nit addressed

  • Updated read_geotiff_gpu band_nodata Parameters entry to state the actual rule (the GPU dispatcher rejects .vrt sources up front) instead of the imprecise "GPU reads do not go through the VRT pipeline" wording. Commit 955b29d.
  • No code change; pure docstring polish.
  • Regression test still passes (16/16 in test_read_entry_points_doc_param_parity_2274.py).

@brendancol
Copy link
Copy Markdown
Contributor Author

Independent review (deep-sweep follow-up pass)

Ran a fresh review against a clean worktree (pr-2275-review) at commit 955b29d.

Blockers

None.

Suggestions

None.

Nits

  • _PARAM_HEADING = r"^(\w+) : " is permissive enough that a future docstring with a prose line like See Also : foo outside the Parameters block would register as a phantom parameter and trip test_read_entry_point_docstring_does_not_invent_params. Current docstrings contain no such lines, so it is not a real problem today; anchoring the scan to the Parameters section would harden the regression test against that one drift class.

Verification

  • test_read_entry_points_doc_param_parity_2274.py — 16/16 pass on Python 3.14.
  • Empirical scan of the four readers post-cleanup: sig == doc exactly (open_geotiff 16/16, read_geotiff_dask 15/15, read_geotiff_gpu 16/16, read_vrt 16/16). No missing or phantom params.
  • Diff is documentation-only as advertised; signatures, defaults, and dispatch logic untouched.
  • The band_nodata rationale on read_geotiff_gpu reads accurately after the 955b29d follow-up.

CI status

All 4 required jobs green (label, pytest ubuntu/macos/windows on 3.14). Readthedocs status is PENDING and is not a required check. mergeStateStatus is BLOCKED pending approval, not on any failing check.

@brendancol brendancol merged commit 5391a27 into main May 22, 2026
5 checks passed
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.

geotiff: backend readers missing Parameters docstring entries for shared kwargs

1 participant