Validate read_vrt window against VRT extent (#1697)#1698
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an inconsistency in the GeoTIFF/VRT read path: read_vrt(path, window=...) no longer silently clamps out-of-range/degenerate windows, and instead raises a ValueError aligned with the local-file and HTTP COG readers. This prevents confusing downstream xarray coordinate-validation failures when caller-built coords don’t match clamped data shapes.
Changes:
- Replace VRT window clamping with explicit window validation against the parsed VRT extent, raising
ValueErrorfor out-of-bounds / zero-size / inverted windows. - Add a dedicated regression test suite covering invalid/valid windows and ensuring error-message “shape” parity with the local TIFF path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/_vrt.py |
Adds explicit window validation (raises instead of clamping) to match cross-backend contract. |
xrspatial/geotiff/tests/test_vrt_window_validation_1697.py |
Adds regression + parity tests for VRT window validation behavior and messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+408
to
+414
| # source reads. Without this, the clamp below silently shrinks an | ||
| # out-of-bounds window and returns a smaller array, which then | ||
| # mismatches caller-built coord arrays in ``open_geotiff`` and | ||
| # surfaces as an opaque ``CoordinateValidationError``. Mirrors the | ||
| # local-path validator in ``read_to_array`` (#1634) and the HTTP | ||
| # path validator in ``_read_cog_http`` (#1669) so all backends | ||
| # agree on the contract. See issue #1697. |
The local TIFF path (#1634) and HTTP COG path (#1669) reject out-of-bounds, zero-size, and inverted windows up front with a ValueError. The VRT path silently clamped the window to the parsed extent instead, which returned a smaller array than open_geotiff's caller-built coord arrays and surfaced as a CoordinateValidationError deep inside xarray. Replace the clamp with the same validator the other two paths use, swapping "source extent" to "VRT extent" since the dimensions come from the parsed VRT rather than a single source file. Fixes #1697
brendancol
force-pushed
the
fix-vrt-window-validation-2026-05-12
branch
from
77e81e0 to
47d5bcc
Compare
The block comment above the window check still referenced "the clamp below", but the clamp logic was removed in #1697. Reword to describe the prior behavior (per-source reads silently clamped out-of-bounds windows) and why we now validate up front, keeping the issue reference so future readers can trace history.
2 tasks
brendancol
added a commit
that referenced
this pull request
May 12, 2026
) PRs #1692 and #1698 raced through main with conflicting expectations for out-of-bounds window=. #1692's tests were written against the pre-#1697 contract that silently clamped windows to the VRT extent (returning a smaller array). #1698 replaced that contract with the validator-rejects-up-front behaviour from #1634 / #1669 so all three backends (local, HTTP, VRT) raise ValueError on out-of-bounds windows. Update both clamp-assertion tests to use pytest.raises(ValueError) and rename them to reflect the new contract. The "negative offsets clamp to 0" path is also gone -- negative offsets now raise.
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
Fixes #1697.
read_vrt(path, window=...)was silently clamping out-of-range windows instead of raisingValueError. The local TIFF path (#1634) and the HTTP COG path (#1669) already reject the same bad windows; the VRT path got missed when #1634 landed.The hidden failure mode is the one #1634 was filed against:
open_geotiffbuilds the y/x coord arrays from the caller-supplied window indices, so a clamped read comes back smaller than the coord arrays and xarray then raisesCoordinateValidationErrorfrom inside its constructor. The user sees an opaque xarray error instead of a clear xrspatial one.This PR replaces the clamp in
xrspatial/geotiff/_vrt.py:407-414with the same validator the other two paths use. Message says "VRT extent" instead of "source extent" since the dimensions come from the parsed VRT rather than a single source file.Before / after on a 4x4 VRT
(-1, 0, 2, 2)(2, 2)arrayValueError(0, 0, 5, 5)(4, 4)arrayValueError(2, 2, 2, 2)(0, 0)arrayValueError(1, 1, 3, 3)(valid)(2, 2)array(2, 2)arrayTest plan
xrspatial/geotiff/tests/test_vrt_window_validation_1697.py-- 15 new tests covering negative starts, past-edge stops, zero-size windows, inverted windows, valid in-bounds regressions, and a cross-path check that the VRT and local-TIFF rejections share the same error class and message shape for the same bad window.pytest xrspatial/geotiff/tests/test_vrt*.py) still green -- 104 passed.