geotiff: refuse VRT non-nearest resample algorithms (#1751)#1761
Open
brendancol wants to merge 1 commit into
Open
geotiff: refuse VRT non-nearest resample algorithms (#1751)#1761brendancol wants to merge 1 commit into
brendancol wants to merge 1 commit into
Conversation
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.
Contributor
There was a problem hiding this comment.
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
NotImplementedErrorfor non-nearest algorithms whenSrcRect/DstRectsizes 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 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. |
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
ComplexSourceparses<ResampleAlg>from VRT XML (_vrt.py:356-360) butread_vrtalways calls_resample_nearestat the placement step (_vrt.py:798-804), so a VRT declaringBilinear/Cubic/CubicSpline/Lanczos/Average/Modesilently received nearest-sampled pixels mislabelled as the requested algorithm -- quietly wrong analytics.NotImplementedErrorat the resample call site whensrc.resample_algis anything other than nearest. The case-insensitive accept list is'',None,Nearest,NearestNeighbour,NearestNeighbor,NEAR.needs_resamplebranch rather than at parse time so aComplexSourcedeclaringBilinearwith matching SrcRect/DstRect sizes is still accepted -- no resample kernel runs, so no mislabelled output.Nearestor wait for follow-up work.Test plan
test_vrt_resample_alg_1751.py(16 cases): each of Bilinear/Cubic/CubicSpline/Lanczos/Average/Mode raises with the algorithm name and1751in the message; case-insensitivebilinearalso raises; Nearest / NearestNeighbour / NearestNeighbor / NEAR / nearest / NEAREST / empty / absent<ResampleAlg>all round-trip;Bilinearat matching SrcRect/DstRect sizes does NOT raise (pins down the resample-site placement).mainwithout the source change and pass with it.pytest xrspatial/geotiff/tests/ -q(1992 pass, 3 skipped; the GPUtest_predictor2_big_endian_gpu_1517andtest_no_georef_windowed_coords_1710failures are pre-existing environment issues, identical on main).