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
@brendancol brendancol deleted the deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-03 branch May 15, 2026 04:40
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