geotiff: GPU stripped windowed coords honour has_georef=False (#1753)#1763
Open
brendancol wants to merge 1 commit into
Open
geotiff: GPU stripped windowed coords honour has_georef=False (#1753)#1763brendancol wants to merge 1 commit into
brendancol wants to merge 1 commit into
Conversation
The GPU stripped fallback path in read_geotiff_gpu (added in PR #1738 so the GPU path forwards max_pixels / window / band to the CPU helper) returned float64 coords synthesised from the default unit GeoTransform for non-georef windowed reads, while the eager numpy, dask, and tiled GPU paths all returned integer pixel coords. The pre-fix output ``y=[-0.5, -1.5, ...]`` regressed the #1710 fix on the stripped GPU fallback only. The branch at __init__.py 2724-2747 only guarded ``t is None``, which is unreachable because _extract_transform always returns a default GeoTransform() instance with has_georef=False. The default transform has pixel_width=1.0, pixel_height=-1.0, so the PixelIsArea sub-branch synthesised the half-pixel-shifted negative-y output instead of falling through to the integer-coord shortcut. dask+cupy stripped windowed reads shared the same code path so the bug fired there too. Fix: route ``not getattr(geo_info, 'has_georef', True)`` through the integer-coord branch alongside the existing ``t is None`` check. Mirror the eager numpy ``open_geotiff`` body at __init__.py:834-836 and the dask path at read_geotiff_dask which both already check has_georef. Also corrected the latent off-by-one in the integer-coord branch (was arange(r1-r0); should be arange(r0, r1) so an offset window like (2,3,6,7) returns [2,3,4,5]/[3,4,5,6] not [0,1,2,3]). 14 regression tests in test_gpu_stripped_no_georef_window_1753.py: - TestStrippedGpuWindowedNoGeoref: origin window, offset window, dask+cupy variants of both, uint16 dtype, no transform attr - TestStrippedGpuWindowedBackendParity: dtype-parity (3 windows) and value-parity (3 windows) across eager / dask / GPU / dask+cupy - TestStrippedGpuWindowedGeorefStillWorks: PixelIsArea origin and offset windows still get float64 coords with the correct half-pixel shift (regression guard against the fix mis-firing on georef files) All 2005 non-stale geotiff tests still pass. 10 pre-existing failures in test_predictor2_big_endian_gpu_1517.py are stale test infrastructure from PR #1708 (which renamed read_to_array to _read_to_array in the public module namespace) and unrelated to this fix. Categories: Cat 3 (off-by-one boundary handling, latent on the previously unreachable branch) + Cat 5 (backend inconsistency: CPU eager and dask returned int64 integer coords while stripped GPU returned float64 half-pixel-shifted coords on the same file). Closes #1753
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
Fix backend inconsistency on
open_geotiff(path, gpu=True, window=...)for non-georef stripped TIFFs. PR #1738 (the GPU stripped fallback that forwardsmax_pixels/window/bandto the CPU helper) regressed the #1710 fix for the GPU stripped fallback path: a non-georef file read withgpu=True, window=...returned float64 coords synthesised from the default unitGeoTransform(e.g.y=[-0.5, -1.5, -2.5, -3.5]), while the eager numpy, dask, and tiled GPU paths returned integer pixel coords (np.arange(c0, c1, dtype=int64)).The fix routes the
has_georef=Falsecase through the integer-coord branch on the GPU stripped fallback, mirroring the eager and dask paths.Root cause
The windowed coord branch added in #1738 only checks
if t is None:. That is unreachable because_extract_transformalways returns a defaultGeoTransform()instance withhas_georef=False. The default transform haspixel_width=1.0,pixel_height=-1.0, so the PixelIsArea sub-branch produced(c0 + 0.5) * 1.0 + 0and(r0 + 0.5) * -1.0 + 0->[-0.5, -1.5, ...].dask+cupyshares the same code path, so the bug fired on everygpu=Truewindowed read of a non-georef stripped TIFF regardless of chunking.Fix
not getattr(geo_info, 'has_georef', True)through the integer-coord branch alongside the existingt is Nonecheck at__init__.py:2724-2747.open_geotifflines 834-836.arange(r1-r0)instead ofarange(r0, r1), so an offset window like(2, 3, 6, 7)would have returned[0, 1, 2, 3]instead of[2, 3, 4, 5]. The branch was unreachable so the off-by-one was latent.Tests
14 new regression tests in
test_gpu_stripped_no_georef_window_1753.py:TestStrippedGpuWindowedNoGeoref: origin window, offset window, dask+cupy variants, uint16 dtype, no transform attrTestStrippedGpuWindowedBackendParity: dtype-parity (3 windows) and value-parity (3 windows) across all four backendsTestStrippedGpuWindowedGeorefStillWorks: PixelIsArea origin and offset windows still get float64 coords with the correct half-pixel shiftTest plan
test_no_georef_windowed_coords_1710.pypass (3 previously failed inTestGpuWindowedCoordsandTestBackendParity).test_predictor2_big_endian_gpu_1517.pyare unrelated stale test-infrastructure from PR geotiff: read_to_array leaks into public namespace but is not in __all__ or docs #1708 (renamedread_to_arrayto_read_to_array).Closes #1753