Skip to content

geotiff: refuse VRT non-nearest resample algorithms (#1751)#1761

Merged
brendancol merged 2 commits into
mainfrom
issue-1751
May 13, 2026
Merged

geotiff: refuse VRT non-nearest resample algorithms (#1751)#1761
brendancol merged 2 commits into
mainfrom
issue-1751

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Closes geotiff: VRT ResampleAlg is parsed but ignored (silent nearest-neighbour fallback) #1751. ComplexSource parses <ResampleAlg> from VRT XML (_vrt.py:356-360) but read_vrt always calls _resample_nearest at the placement step (_vrt.py:798-804), so a VRT declaring Bilinear / Cubic / CubicSpline / Lanczos / Average / Mode silently received nearest-sampled pixels mislabelled as the requested algorithm -- quietly wrong analytics.
  • Raise NotImplementedError at the resample call site when src.resample_alg is anything other than nearest. The case-insensitive accept list is '', None, Nearest, NearestNeighbour, NearestNeighbor, NEAR.
  • The check sits inside the needs_resample branch rather than at parse time so a ComplexSource declaring Bilinear with matching SrcRect/DstRect sizes is still accepted -- no resample kernel runs, so no mislabelled output.
  • Implementing the higher-quality kernels is out of scope for this PR; the goal is to stop the silent wrong-numbers behaviour. Error message names the requested algorithm and points at issue geotiff: VRT ResampleAlg is parsed but ignored (silent nearest-neighbour fallback) #1751 so a user can re-export with Nearest or wait for follow-up work.

Test plan

  • New test_vrt_resample_alg_1751.py (16 cases): each of Bilinear/Cubic/CubicSpline/Lanczos/Average/Mode raises with the algorithm name and 1751 in the message; case-insensitive bilinear also raises; Nearest / NearestNeighbour / NearestNeighbor / NEAR / nearest / NEAREST / empty / absent <ResampleAlg> all round-trip; Bilinear at matching SrcRect/DstRect sizes does NOT raise (pins down the resample-site placement).
  • Confirmed all 16 tests fail on main without the source change and pass with it.
  • pytest xrspatial/geotiff/tests/ -q (1992 pass, 3 skipped; the GPU test_predictor2_big_endian_gpu_1517 and test_no_georef_windowed_coords_1710 failures are pre-existing environment issues, identical on main).

ComplexSource parses <ResampleAlg> from VRT XML but read_vrt always
calls _resample_nearest in the placement step, so a VRT declaring
Bilinear / Cubic / CubicSpline / Lanczos / Average / Mode silently
received nearest-sampled pixels mislabelled as the requested algorithm.

Raise NotImplementedError at the resample call site when src.resample_alg
is not nearest (case-insensitive: '', None, 'Nearest', 'NearestNeighbour',
'NearestNeighbor', 'NEAR' all pass).  The check sits in the
``needs_resample`` branch rather than at parse time so a ComplexSource
declaring Bilinear with matching SrcRect/DstRect sizes is still accepted
-- no kernel runs, so no mislabelled output.  Implementing the
higher-quality kernels is out of scope here; the goal is to stop the
silent wrong-numbers behaviour.

Closes #1751.
@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 23:50
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

Prevents read_vrt from silently returning nearest-neighbour sampled pixels when a VRT ComplexSource declares a non-nearest <ResampleAlg> and resampling is actually required, addressing issue #1751.

Changes:

  • Add a resample-algorithm support check that raises NotImplementedError for non-nearest algorithms when SrcRect/DstRect sizes differ.
  • Define and use a normalized allowlist of “nearest” algorithm variants (Nearest*, NEAR, empty/absent).
  • Add regression tests covering unsupported algorithms, case-insensitivity, nearest variants, missing <ResampleAlg>, and the “no resample needed” case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
xrspatial/geotiff/_vrt.py Refuses non-nearest <ResampleAlg> at the resample call site to avoid silently wrong output.
xrspatial/geotiff/tests/test_vrt_resample_alg_1751.py Adds regression coverage for supported/unsupported <ResampleAlg> values and the conditional behavior.
Comments suppressed due to low confidence (1)

xrspatial/geotiff/_vrt.py:364

  • This comment says the resample site 'refuses the read for any other declared algorithm', but in the implementation the refusal only happens when a resample is actually needed (SrcRect/DstRect sizes differ). Please tweak the wording to reflect the conditional behavior so the docs match the code path.
                # ``<ResampleAlg>`` records the requested resampler for
                # the placement step.  ``read_vrt`` only implements
                # nearest-neighbour today, so the resample site refuses
                # the read for any other declared algorithm rather than
                # returning silently-mislabelled pixels.  See issues
                # #1694 and #1751.
                resample_alg = _text(src_elem, 'ResampleAlg')

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

Comment thread xrspatial/geotiff/_vrt.py Outdated
Comment on lines +154 to +157
# the placement step (issue #1694); the resample site raises
# ``NotImplementedError`` for any other declared algorithm rather
# than silently substituting nearest (issue #1751). Higher-quality
# resamplers are tracked for follow-up.
Reword both comments so they accurately describe the conditional
behavior: NotImplementedError fires only when SrcRect and DstRect sizes
differ.  Sources with matching rect sizes pass through regardless of
the declared algorithm because no resampling is required.
@brendancol
Copy link
Copy Markdown
Contributor Author

Clarified both comments in 4bf143f. They now state that the NotImplementedError fires only when SrcRect and DstRect sizes differ (resampling actually required), and that a non-nearest alg with matching rect sizes is a no-op that passes through.

@brendancol brendancol merged commit 42cec2b into main May 13, 2026
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: VRT ResampleAlg is parsed but ignored (silent nearest-neighbour fallback)

2 participants