Skip to content

Add VRT support contract docs (#2368)#2374

Open
brendancol wants to merge 3 commits into
mainfrom
issue-2368
Open

Add VRT support contract docs (#2368)#2374
brendancol wants to merge 3 commits into
mainfrom
issue-2368

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2368.
Part of epic #2342 (PR 5 of 5).

Summary

  • Document VRT as a conservative advanced feature for simple GeoTIFF mosaics, not a full GDAL VRT replacement.
  • Adds matching "Supported" and "Not supported / Not promised" lists to: the internal read_vrt docstring in xrspatial/geotiff/_vrt.py, and a new "VRT support contract" section in docs/source/reference/geotiff.rst placed right before the existing "VRT missing sources" section.
  • Adds a short pointer (no duplicate lists) to the public read_vrt docstring in xrspatial/geotiff/_backends/vrt.py and to open_geotiff in xrspatial/geotiff/__init__.py.
  • Adds a one-line note with a link to the contract section in the README's GeoTIFF / COG I/O section.

Scope

Docs and docstrings only. No production logic changes. The validator behaviour, positive tests, negative tests, and missing-source policy tests are tracked in sibling PRs under epic #2342.

Backend coverage

N/A. This PR does not touch runtime code.

Test plan

  • python -c "import xrspatial.geotiff" and python -c "from xrspatial.geotiff import open_geotiff, read_vrt" succeed with the new docstrings present.
  • ast.parse passes on every edited .py file.
  • Sphinx build picks up the new section (CI will exercise the docs build if the workflow runs it).
  • README link to docs/source/reference/geotiff.rst#vrt-support-contract resolves on GitHub's Markdown view.

Document VRT as a conservative advanced feature for simple GeoTIFF
mosaics rather than full GDAL VRT compatibility. Adds the supported
subset and out-of-scope lists from epic #2342 to the read_vrt
docstring in xrspatial/geotiff/_vrt.py, a pointer in the public
read_vrt and open_geotiff docstrings, a "VRT support contract"
section in docs/source/reference/geotiff.rst, and a one-line note in
the README's GeoTIFF / COG I/O section.

Docs and docstrings only. No production logic changes.

Closes #2368.
Part of #2342.
@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: Add VRT support contract docs (#2368)

Docs-only PR. Steps 2 (numerical correctness), 3 (backend dispatch), and 4 (perf) don't apply. The review below focuses on docstring layout, link correctness, and consistency with epic #2342.

Blockers

None.

Suggestions

  • README link docs/source/reference/geotiff.rst#vrt-support-contract (README.md:153) relies on GitHub's RST renderer producing the slug vrt-support-contract for the VRT support contract section. That's the common case but not guaranteed across all renderers. Worth a manual check once the PR is rendered; if it doesn't resolve to the section, fall back to the bare file path or to a published Sphinx URL.
  • xrspatial/geotiff/_vrt.py:1011-1044: the new "Supported subset" and "Not supported" blocks sit between the mask_nodata Parameters entry and Returns. Napoleon (which this project uses for Sphinx) handles this fine. Strict numpydoc-style readers warn about unknown sections. Moving the contract into a Notes section right after Returns would parse cleanly under both. Also worth coordinating with sibling PR 1 of epic #2342, which is editing the same file.

Nits

  • xrspatial/geotiff/_vrt.py:1013 reads "VRT here is a conservative advanced feature...". The rst and the __init__.py snippet both lead with "The VRT path is...". Matching the three intros would make the contract look like one voice rather than three.
  • docs/source/reference/geotiff.rst:190: the new label .. _reference.geotiff.vrt_support_contract: uses dot-separated segments. The rest of the file does not follow one local convention strictly, so this is fine; flagging only in case there is a preference elsewhere in docs/.
  • The Markdown anchor (#vrt-support-contract) and the Sphinx label (vrt_support_contract) target different systems but look like a near-miss to a skim-reader. Not worth fixing, just noting.

What looks good

  • Lists in the four touched files match epic #2342 verbatim.
  • The internal _vrt.read_vrt carries the full lists; the public backend read_vrt and open_geotiff carry only pointers, matching the brief's "do not duplicate".
  • Placement of the new rst section right before "VRT missing sources" makes the contract read first and the policy detail second, which is the right order for a first-time user.
  • README link points at the GitHub-renderable rst path, not a docs URL that may not exist yet.

Checklist (docs-only)

  • Lists match epic #2342 in all four locations
  • Docstrings parse (ast.parse clean, runtime __doc__ populated)
  • Sphinx (Napoleon) accepts the new docstring layout
  • README link target file exists
  • No production logic changes
  • [n/a] Backend dispatch, NaN handling, dask correctness, benchmark coverage

Move the "Supported subset" / "Not supported" blocks in
xrspatial/geotiff/_vrt.py:read_vrt out from between Parameters and
Returns and into a proper Notes section. Strict numpydoc-style
readers warn on unknown section headers; the Notes section is a
standard numpydoc/Napoleon header that allows free-form prose plus
inline-emphasis subsection labels.

Also aligns the intro line with the public read_vrt and open_geotiff
docstrings ("The VRT path is a conservative advanced feature...") so
the three surfaces read as one voice.

Reviewer dispositions:
- Fix: numpydoc parse cleanliness (this commit)
- Fix: consistent intro phrasing (this commit)
- Defer: README anchor verification (needs rendered PR view)
- Dismiss: label naming, anchor near-miss (reviewer flagged as
  non-issues)
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 review after f8c3140

Disposition of the original review findings:

Fixed (in f8c3140):

  • Suggestion 2 (numpydoc parse cleanliness): the "Supported subset" / "Not supported" blocks in xrspatial/geotiff/_vrt.py:read_vrt are now inside a proper Notes section after Returns, with the subsection labels rendered as inline-emphasis (*Supported:*, *Not supported.*) rather than as ad-hoc headings. Confirmed locally: numpydoc.docscrape.NumpyDocString parses with no UserWarning, and the seven Parameters + the single unnamed Returns entry are still recognised.
  • Nit 1 (intro phrasing): the intro now reads "The VRT path is a conservative advanced feature scoped to simple GeoTIFF mosaics..." matching the rst section and the open_geotiff snippet word for word, so the three surfaces are consistent.

Deferred:

  • Suggestion 1 (README anchor verification): no production-side fix is appropriate without rendering the PR on GitHub to confirm whether docs/source/reference/geotiff.rst#vrt-support-contract resolves to the new section. The link target file is correct, so the worst case is a non-anchored scroll to the top of the file rather than a 404. If the rendered view shows the anchor doesn't resolve, the fix is a follow-up commit changing the README to the bare file path or to a published Sphinx URL once docs are deployed.

Dismissed:

  • Nit 2 (rst label naming convention): the .. _reference.geotiff.vrt_support_contract: label uses dot-separated segments. The reviewer noted the rest of the file does not follow one strict convention, so there is nothing to align against.
  • Nit 3 (Markdown anchor vs Sphinx label near-miss): the two targets are unrelated systems and the reviewer flagged this as "not worth fixing".

No new blockers.

# Conflicts:
#	docs/source/reference/geotiff.rst
#	xrspatial/geotiff/__init__.py
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.

VRT docs: describe conservative simple-mosaic subset and non-goals (#2342 work item)

1 participant