Skip to content

write_vrt: accept crs= for parity with to_geotiff/write_geotiff_gpu (#1715)#1728

Merged
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-03
May 12, 2026
Merged

write_vrt: accept crs= for parity with to_geotiff/write_geotiff_gpu (#1715)#1728
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-03

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

write_vrt was the only writer in xrspatial.geotiff using crs_wkt instead of crs:

to_geotiff(data, path, *, crs: int | str | None = None, ...)
write_geotiff_gpu(data, path, *, crs: int | str | None = None, ...)
write_vrt(vrt_path, source_files, *, crs_wkt: str | None = None, ...)

This broke the "forward the same kwargs to whichever writer matches the output extension" pattern. Generic write-wrapper code had to special-case VRT, convert EPSG int -> WKT for that path, and remember that one writer in the trio used a different name. Reader-side attrs['crs'] is the canonical EPSG-int surface, so the asymmetry is also visible in round-trip code.

This PR adds crs as the canonical kwarg with the same int | str | None shape as the sibling writers. crs_wkt is kept as a deprecated alias for backward compatibility:

  • crs= accepts int (EPSG), str (WKT or PROJ), or None.
  • crs_wkt= still works but emits DeprecationWarning.
  • Passing both raises TypeError rather than silently picking one.
  • Omitting both (the most common call shape) emits no warning.

The wrapper normalises any non-None input through the new _resolve_crs_to_wkt helper before forwarding to the internal _vrt.write_vrt (which only speaks WKT). EPSG int and PROJ-string inputs are routed through pyproj's CRS.from_user_input(...).to_wkt(); already-WKT strings (recognised by the PROJCS/GEOGCS/PROJCRS/GEOGCRS/COMPD_CS/COMPOUNDCRS prefix) pass through verbatim so the existing WKT-passthrough behaviour for the XML-escape tests is preserved.

The deprecation shim follows the same sentinel-default pattern the deprecated gpu= alias uses on read_geotiff_gpu: a private _CRS_WKT_DEPRECATED_SENTINEL distinguishes "user passed nothing" (silent) from "user passed crs_wkt=None explicitly" (warn).

Deprecation impact

This is a non-breaking change. crs_wkt continues to work; it emits a DeprecationWarning on use. Existing user code keeps working unchanged; new code should migrate to crs=. The deprecation note follows the same shape as the existing read_geotiff_gpu(gpu=...) -> read_geotiff_gpu(on_gpu_failure=...) rename.

Closes #1715.

Test plan

  • pytest xrspatial/geotiff/tests/test_write_vrt_crs_1715.py covers signature pins, EPSG-int / WKT-string / None / no-kwarg, deprecation warnings, both-kwargs-raises-TypeError, cross-writer annotation parity, and invalid-type / unparseable-string error paths (12 tests).
  • pytest xrspatial/geotiff/tests/test_polish_1488.py xrspatial/geotiff/tests/test_kwarg_behaviour_2026_05_12.py xrspatial/geotiff/tests/test_signature_parity_1631.py xrspatial/geotiff/tests/test_signature_annotations_1654.py all pass with the test-side migrations to crs=.
  • pytest xrspatial/geotiff/tests/test_vrt_xml_escape_1607.py still passes (uses the internal _vrt.write_vrt, not the public wrapper -- unaffected).
  • Smoke: write_vrt + read_vrt round-trip on NumPy / Dask / CuPy backends with crs=4326.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 12, 2026
@brendancol brendancol requested a review from Copilot May 12, 2026 19:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the public xrspatial.geotiff.write_vrt API to accept a crs= kwarg (matching to_geotiff / write_geotiff_gpu) while keeping crs_wkt= as a deprecated alias, and adds tests to pin the new behavior and signature parity across writers.

Changes:

  • Added crs: int | str | None to write_vrt, with crs_wkt retained as a deprecated alias (warns on use; passing both raises TypeError).
  • Introduced _resolve_crs_to_wkt to normalize EPSG ints / PROJ strings to WKT before calling the internal VRT writer.
  • Added/updated tests to cover signature parity, deprecation warnings, and CRS round-trips.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
xrspatial/geotiff/__init__.py Implements write_vrt(crs=...), deprecated crs_wkt alias handling, and CRS-to-WKT normalization helper.
xrspatial/geotiff/tests/test_write_vrt_crs_1715.py New regression tests covering the new crs= kwarg, deprecation behavior, and error paths.
xrspatial/geotiff/tests/test_signature_parity_1631.py Updates signature-parity assertions to include the new crs kwarg and sentinel default behavior.
xrspatial/geotiff/tests/test_polish_1488.py Migrates usage from crs_wkt=None to crs=None to avoid deprecation warnings.
xrspatial/geotiff/tests/test_kwarg_behaviour_2026_05_12.py Updates behavior tests to use crs= and documents the rename/deprecation.
Comments suppressed due to low confidence (1)

xrspatial/geotiff/tests/test_write_vrt_crs_1715.py:225

  • The test docstring claims the WKT keyword heuristic "recognises PROJCS/GEOGCS only", but _resolve_crs_to_wkt also treats other WKT roots as passthrough (e.g., PROJCRS/GEOGCRS/COMPD_CS/COMPOUNDCRS). Update the wording to match the implementation so the test remains accurate documentation.
    """``crs='not a CRS'`` raises ``ValueError`` from the public
    wrapper (the WKT keyword heuristic recognises PROJCS/GEOGCS only;
    everything else is sent through pyproj which will reject it)."""

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +31 to +36
from xrspatial.geotiff import (
open_geotiff,
read_vrt,
to_geotiff,
write_vrt,
)

Uses the new ``crs=None`` kwarg form (issue #1715). The deprecated
``crs_wkt`` alias is exercised separately in
``test_namespace_no_leak_1715.py``.
Comment thread xrspatial/geotiff/__init__.py Outdated
Comment on lines +3563 to +3567
crs_wkt : str, optional
Deprecated alias for ``crs``. Emits ``DeprecationWarning`` when
used. Passing both ``crs`` and ``crs_wkt`` raises ``TypeError``.
Accepts a WKT string only (the historic surface); use ``crs``
for EPSG int or PROJ string input.
…1715)

The api-consistency sweep on 2026-05-12 found ``write_vrt`` was the only
writer in ``xrspatial.geotiff`` using ``crs_wkt`` instead of ``crs``:

    to_geotiff(data, path, *, crs: int | str | None = None, ...)
    write_geotiff_gpu(data, path, *, crs: int | str | None = None, ...)
    write_vrt(vrt_path, source_files, *, crs_wkt: str | None = None, ...)

This broke the "forward the same kwargs to whichever writer matches the
output extension" pattern. Generic write-wrapper code had to special-case
VRT, convert EPSG int -> WKT for that path, and remember that one writer
in the trio used a different name. Reader-side ``attrs['crs']`` is the
canonical EPSG-int surface, so the asymmetry is also visible in
round-trip code.

This change adds ``crs`` as the canonical kwarg with the same
``int | str | None`` shape as the sibling writers. ``crs_wkt`` is kept
as a deprecated alias for backward compatibility:

* ``crs=`` accepts int (EPSG), str (WKT or PROJ), or None.
* ``crs_wkt=`` still works but emits ``DeprecationWarning``.
* Passing both raises ``TypeError`` rather than silently picking one.
* Omitting both (the most common call shape) emits NO warning.

The wrapper normalises any non-None input through the new
``_resolve_crs_to_wkt`` helper before forwarding to the internal
``_vrt.write_vrt`` (which only speaks WKT). EPSG int and PROJ-string
inputs are routed through pyproj's ``CRS.from_user_input(...).to_wkt()``;
already-WKT strings (recognised by the PROJCS/GEOGCS/PROJCRS/GEOGCRS/
COMPD_CS/COMPOUNDCRS prefix) pass through verbatim so the existing
WKT-passthrough behaviour for the XML-escape tests is preserved.

The deprecation shim follows the same sentinel-default pattern the
deprecated ``gpu=`` alias uses on ``read_geotiff_gpu``: a private
``_CRS_WKT_DEPRECATED_SENTINEL`` distinguishes "user passed nothing"
(silent) from "user passed ``crs_wkt=None`` explicitly" (warn).

test_write_vrt_crs_1715.py adds 12 tests pinning the new contract:

* signature pins (``crs`` present, default None, annotation
  ``int | str | None``);
* runtime: ``crs=<EPSG int>``, ``crs=<WKT string>``, ``crs=None``,
  no-crs-kwarg-no-warning, all round-trip back to ``attrs['crs']``;
* deprecation shim: ``crs_wkt=<wkt>`` warns + still works,
  ``crs_wkt=None`` (explicit) warns, both-kwargs raises ``TypeError``;
* cross-writer parity: ``crs`` annotation matches on all three writers;
* negative tests: invalid type / unparseable string raise on the public
  wrapper.

Existing tests that were calling ``write_vrt(..., crs_wkt=...)`` through
the public wrapper are migrated to ``crs=`` so the suite no longer emits
spurious DeprecationWarnings:

* ``test_polish_1488.py``: ``crs=None`` replaces ``crs_wkt=None``.
* ``test_kwarg_behaviour_2026_05_12.py::TestWriteVrtCrsWktBehaviour``:
  three sub-tests migrated from ``crs_wkt=`` to ``crs=``.
* ``test_signature_parity_1631.py``: signature test updated to assert
  both kwargs and uses ``crs=None`` in the runtime call.

``test_vrt_xml_escape_1607.py`` keeps using ``crs_wkt=`` because it
imports from the internal ``xrspatial.geotiff._vrt`` (not the public
wrapper) and exercises raw XML-escape semantics; the rename does not
apply there.

Closes #1715.
@brendancol brendancol force-pushed the deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-03 branch from b337d38 to b8c5fe7 Compare May 12, 2026 19:29
- Drop unused ``open_geotiff`` import from test_write_vrt_crs_1715
- Fix stale ``test_namespace_no_leak_1715.py`` reference in the
  test_signature_parity_1631 docstring; the deprecated ``crs_wkt``
  alias is exercised in test_write_vrt_crs_1715.py
- Correct write_vrt ``crs_wkt`` docstring: the alias is ``str | None``
  and runs through ``_resolve_crs_to_wkt`` (so WKT, PROJ strings, and
  ``EPSG:NNNN`` all work), not WKT-only
@brendancol brendancol merged commit 09ac06a into main May 12, 2026
10 of 11 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: write_vrt's crs_wkt kwarg drifts from to_geotiff/write_geotiff_gpu's crs

2 participants