Skip to content

Reference docs gap-fill in geotiff.rst (#2345 PR 2) — closes #2380#2387

Merged
brendancol merged 2 commits into
mainfrom
issue-2380
May 25, 2026
Merged

Reference docs gap-fill in geotiff.rst (#2345 PR 2) — closes #2380#2387
brendancol merged 2 commits into
mainfrom
issue-2380

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

Fills the five gaps the #2345 release-gate audit checklist flagged on docs/source/reference/geotiff.rst. Five sections added:

  • Nodata lifecycle. Brief summary of integer, float, NaN, and masked_nodata behaviour. Cross-links to user_guide.attrs_contract for the full lifecycle so this page does not duplicate it.
  • Rotated and sheared transforms. Read posture (allow_rotated=True drops crs), write posture (drop_rotation=True opt-in), and the combinations that fail closed.
  • Remote-read safety limits. Consolidated env-var reference for max_cloud_bytes, XRSPATIAL_GEOTIFF_MAX_CLOUD_BYTES, XRSPATIAL_COG_MAX_TILE_BYTES, XRSPATIAL_GEOTIFF_HTTP_CONNECT_TIMEOUT / _READ_TIMEOUT, XRSPATIAL_GEOTIFF_ALLOW_PRIVATE_HOSTS, and XRSPATIAL_VRT_ALLOWED_ROOTS, each pointing at the regression test that locks the knob.
  • GPU support. Explicit experimental statement, the tier each path reports in SUPPORTED_FEATURES, and a "what to expect / what not to rely on" split.
  • Known unsupported combinations. A matrix that mirrors the negative rows in release_gate_geotiff.rst, with every combo pointing at the regression test that locks it (cog=True, tiled=False, warped VRT, nested VRT, mixed-CRS VRT, rotated without the opt-in, and so on).

Scope

  • Docs only (docs/source/reference/geotiff.rst).
  • No changes to xrspatial/geotiff/, no new tests, no API changes.
  • Acceptance: every required section is on the page and reachable via the sidebar TOC; each names or links the regression test that backs it; nothing contradicts the tier table, SUPPORTED_FEATURES, or release_gate_geotiff.rst.

Test plan

  • Sphinx docs CI builds the page without new warnings.
  • :ref: and test-file paths resolve (every cited test file is in xrspatial/geotiff/tests/ -- spot-checked locally).
  • The five new section headings show up in the sidebar TOC.

Closes #2380. Part of epic #2345.

Add the five sections the release-gate audit checklist promised but the
reference page did not surface yet:

* Nodata lifecycle (cross-links to user_guide.attrs_contract for the
  full lifecycle so the page does not duplicate that contract)
* Rotated and sheared transforms (read posture, write posture, and
  the failure-closed combinations)
* Remote-read safety limits with the full env-var quick reference and
  the regression test behind each knob
* GPU support, explicitly tagged experimental, with the SUPPORTED_FEATURES
  tier each path reports and what users should and should not expect
* Known unsupported combinations, as a matrix that names every combo
  and the regression test that locks it

Each row points to the regression test that backs it. None of the new
prose contradicts the existing tier table, SUPPORTED_FEATURES, or
release_gate_geotiff.rst.

Closes #2380.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 25, 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: Reference docs gap-fill in geotiff.rst (#2345 PR 2)

Blockers (must fix before merge)

  • Mis-attributed test function names. Four test functions cited as living in xrspatial/geotiff/tests/test_vrt_unsupported_2370.py actually live in xrspatial/geotiff/tests/test_unsupported_features_2349.py. Specifically:
    • "Rotated write" prose paragraph -- test_eager_writer_rejects_rotated_6tuple_transform and test_vrt_with_skewed_geotransform_rejected are pointed at test_vrt_unsupported_2370.py; real location is test_unsupported_features_2349.py.
    • Known unsupported combinations table, "Mixed per-band nodata across VRT sources" row -- test_mixed_per_source_nodata_rejected pointed at test_vrt_unsupported_2370.py; real location is test_unsupported_features_2349.py.
    • Known unsupported combinations table, "Rotated write without drop_rotation=True" row -- both test_eager_writer_rejects_rotated_6tuple_transform and test_eager_writer_rejects_rotated_affine_attr pointed at test_vrt_unsupported_2370.py; real location is test_unsupported_features_2349.py.
    • Known unsupported combinations table, "Skewed VRT geotransform" row -- test_vrt_with_skewed_geotransform_rejected pointed at test_vrt_unsupported_2370.py; real location is test_unsupported_features_2349.py.
    • This blocks merge because the whole point of the audit trail is that prose-to-test pointers resolve. A maintainer following any of these citations to verify the regression hits a dead end. Fix: swap test_vrt_unsupported_2370.py for test_unsupported_features_2349.py at each citation above.

Suggestions

  • Vague test citation for the degenerate-axis row. The "Known unsupported combinations" table currently says "locked by the issue #2214 regression suite" instead of naming a file. A named regression test exists: xrspatial/geotiff/tests/test_degenerate_pixel_size_2214.py. The table's stated rule is "every row names the regression test", so this row should cite the file rather than the issue number.
  • GPU-codec claim is broader than the regression test that backs it. The GPU section says allow_experimental_codecs "does NOT widen GPU support" and codecs outside the GPU set "fall back to CPU even on a GPU read." The fall-back-to-CPU behaviour is locked for the WRITER by test_gpu_writer_cpu_fallback_codecs_2026_05_12.py. The read-side phrasing is broader than that. Either cite the writer test, or soften "even on a GPU read" so the prose only claims what the regression locks.

Nits

  • Heading-level boundary for the new "Remote-read safety limits and env vars" subsection. It's added with --- under "Security and I/O limits" (===), so it sits as a sibling of "HTTP SSRF defenses". That's internally consistent. Worth a quick sanity check during Sphinx render so a reader scanning the sidebar doesn't read it as a top-level peer of "GPU support".
  • Pre-existing label oddity (not introduced by this PR, just worth flagging). The user_guide.attrs_contract label is defined as .. _user_guide.attrs_contract: (two spaces). Sphinx resolves it either way, but the form is non-idiomatic.

What looks good

  • All 36 cited test files resolve.
  • Cross-link to user_guide.attrs_contract is the right boundary -- the lifecycle prose in this PR is a brief, not a duplicate.
  • "Known unsupported combinations" matrix mirrors the negative rows in release_gate_geotiff.rst and adds rotated-write and degenerate-axis rows that were prose-only before.
  • GPU section names the experimental tier and the SUPPORTED_FEATURES keys, so it agrees with _attrs.py:319,331 and with the GPU rows in release_gate_geotiff.rst.
  • No contradictions found against the existing tier table or the stable-COG section.

Checklist

  • Cited regression test files exist (36/36)
  • Cited test function names resolve to the cited files (4 misattributions, see Blockers)
  • Cross-references resolve to defined labels
  • Tier strings match SUPPORTED_FEATURES
  • No contradiction with release_gate_geotiff.rst
  • No contradiction with the stable-COG section
  • User-guide attrs contract cross-linked rather than duplicated
  • RST parses (Sphinx-only directives / roles excluded from validation)

Self-review on PR #2387 flagged four test functions cited against the
wrong test file. Each lives in test_unsupported_features_2349.py,
not test_vrt_unsupported_2370.py:

* test_eager_writer_rejects_rotated_6tuple_transform
* test_eager_writer_rejects_rotated_affine_attr
* test_mixed_per_source_nodata_rejected
* test_vrt_with_skewed_geotransform_rejected

Also:

* Replace the vague "issue #2214 regression suite" pointer on the
  degenerate-axis row with the actual test file
  (test_degenerate_pixel_size_2214.py).
* Narrow the GPU-codec claim so it matches what the regression test
  actually locks: the GPU writer routes unsupported codecs through a
  CPU fallback inside write_geotiff_gpu, locked by
  test_gpu_writer_cpu_fallback_codecs_2026_05_12.py.
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 (pass 2): Reference docs gap-fill in geotiff.rst (#2345 PR 2)

Blockers

None.

Suggestions

None. Pass-1 items have all been addressed:

  • Mis-attributed test function names (Blocker, pass 1): fixed. All four citations now point to test_unsupported_features_2349.py. Verified by walking every <file> ... (<func>) pair in the file with a regex check; every function citation now resolves to the cited file.
  • Vague degenerate-axis citation (Suggestion, pass 1): fixed. The row cites test_degenerate_pixel_size_2214.py directly.
  • GPU-codec claim (Suggestion, pass 1): narrowed. The prose now describes the writer-side CPU fallback and cites test_gpu_writer_cpu_fallback_codecs_2026_05_12.py.

Nits

Same two nits as pass 1, deliberately not changed:

  • "Remote-read safety limits and env vars" is a --- subsection under "Security and I/O limits" (sibling of "HTTP SSRF defenses"). The placement is internally consistent and changing it would mean restructuring the parent section.
  • Pre-existing .. _user_guide.attrs_contract: label form (double space). Not introduced by this PR.

What looks good

  • 36/36 cited regression-test files exist.
  • All function-to-file pairings resolve.
  • Cross-references to reference.geotiff_release_gate, reference.geotiff_release_contract, and user_guide.attrs_contract all hit defined labels.
  • No new contradictions with the tier table, SUPPORTED_FEATURES, or release_gate_geotiff.rst.

Checklist

  • Cited regression test files exist (36/36)
  • Cited test function names resolve to the cited files
  • Cross-references resolve to defined labels
  • Tier strings match SUPPORTED_FEATURES
  • No contradiction with release_gate_geotiff.rst
  • No contradiction with the stable-COG section
  • User-guide attrs contract cross-linked rather than duplicated
  • RST parses (Sphinx-only directives / roles excluded from validation)

Ready to merge from a review standpoint.

@brendancol
Copy link
Copy Markdown
Contributor Author

CI diagnostic: the three failing pytest matrix jobs (ubuntu-latest 3.14, macos-latest 3.14, windows-latest 3.14) fail on xrspatial/geotiff/tests/test_vrt_unsupported_2370.py::test_unsupported_resample_alg_raises[*] (5 parametrized subtests, all expecting NotImplementedError but getting VRTUnsupportedError from the centralised validator in PR #2371). This is a pre-existing test-vs-code mismatch on main -- the latest pytest run on main at SHA 9cd54f6e is also failure, and this PR is docs-only (a single .rst file). Not introduced here, and the sibling test test_unsupported_resample_alg_open_geotiff already accepts either exception, so the fix is to broaden the parametrized test the same way.

@brendancol brendancol merged commit cdcd491 into main May 25, 2026
3 of 7 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.

Reference docs gap-fill in geotiff.rst (#2345 PR 2)

1 participant