write_vrt: accept crs= for parity with to_geotiff/write_geotiff_gpu (#1715)#1728
Merged
brendancol merged 2 commits intoMay 12, 2026
Conversation
Contributor
There was a problem hiding this comment.
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 | Nonetowrite_vrt, withcrs_wktretained as a deprecated alias (warns on use; passing both raisesTypeError). - Introduced
_resolve_crs_to_wktto 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_wktalso 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 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
force-pushed
the
deep-sweep-api-consistency-geotiff-2026-05-12-1778611459-2954-03
branch
from
b337d38 to
b8c5fe7
Compare
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
write_vrtwas the only writer inxrspatial.geotiffusingcrs_wktinstead ofcrs: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
crsas the canonical kwarg with the sameint | str | Noneshape as the sibling writers.crs_wktis kept as a deprecated alias for backward compatibility:crs=accepts int (EPSG), str (WKT or PROJ), or None.crs_wkt=still works but emitsDeprecationWarning.TypeErrorrather than silently picking one.The wrapper normalises any non-None input through the new
_resolve_crs_to_wkthelper before forwarding to the internal_vrt.write_vrt(which only speaks WKT). EPSG int and PROJ-string inputs are routed through pyproj'sCRS.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 onread_geotiff_gpu: a private_CRS_WKT_DEPRECATED_SENTINELdistinguishes "user passed nothing" (silent) from "user passedcrs_wkt=Noneexplicitly" (warn).Deprecation impact
This is a non-breaking change.
crs_wktcontinues to work; it emits aDeprecationWarningon use. Existing user code keeps working unchanged; new code should migrate tocrs=. The deprecation note follows the same shape as the existingread_geotiff_gpu(gpu=...)->read_geotiff_gpu(on_gpu_failure=...)rename.Closes #1715.
Test plan
pytest xrspatial/geotiff/tests/test_write_vrt_crs_1715.pycovers 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.pyall pass with the test-side migrations tocrs=.pytest xrspatial/geotiff/tests/test_vrt_xml_escape_1607.pystill passes (uses the internal_vrt.write_vrt, not the public wrapper -- unaffected).write_vrt + read_vrtround-trip on NumPy / Dask / CuPy backends withcrs=4326.