Skip to content

geotiff tests: consolidate reader-path cluster#2412

Merged
brendancol merged 3 commits into
mainfrom
issue-2401
May 26, 2026
Merged

geotiff tests: consolidate reader-path cluster#2412
brendancol merged 3 commits into
mainfrom
issue-2401

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2401. PR 8 of epic #2390.

Summary

Folds 13 reader-side GeoTIFF test files into eight responsibility-grouped modules under xrspatial/geotiff/tests/read/:

  • read/test_basic.py -- band validation on read.
  • read/test_dtypes.py -- dtype kwarg, float16 auto-promotion, GPU dtype paths.
  • read/test_compression.py -- codec round-trips and decompression-bomb caps.
  • read/test_tiling.py -- per-tile / per-strip byte caps on CPU and GPU.
  • read/test_endianness.py -- big-endian multi-byte GPU read.
  • read/test_nodata.py -- GPU nodata mask helper.
  • read/test_coords.py -- descending / ascending coord round-trip.
  • read/test_streaming.py -- streaming BigTIFF threshold.

The one cross-directory move is xrspatial/tests/test_geotiff_streaming_bigtiff_threshold_1785.py, which folds into read/test_streaming.py per the epic.

Tests-only restructure; no source changes. Bodies are preserved except for: dropped issue numbers in names, class grouping for related cases, fixture filenames stripped of issue numbers, and relative-import paths adjusted for the new depth.

CLUSTER_AUDIT_PR8.md maps every old file::test to its new home and gets deleted in a follow-up commit on the same branch before merge.

Backend coverage

Reader paths are exercised across numpy, cupy, dask+numpy, and dask+cupy via the existing GPU-only / dask-only markers. GPU and tifffile-backed cases skip cleanly when those dependencies are absent.

Parallel-PR collision heads-up

PR 3 (rotated CRS) creates read/__init__.py and read/test_crs.py in parallel. Whichever PR merges second resolves the trivial __init__.py conflict; __init__.py is intentionally empty for that reason.

Test plan

  • pytest xrspatial/geotiff/tests/read/ -v -- 134 collected, 132 passed, 2 skipped (gated by tifffile/cupy availability).
  • pytest xrspatial/geotiff/tests/ -x -q -- 5716 passed, 68 skipped, 6 xfailed.
  • pytest xrspatial/tests/ -x -q -- 4039 passed, 15 skipped (confirms the cross-directory move did not leave stranded imports).
  • Docs cross-references in geotiff.rst and release_gate_geotiff.rst updated to the new paths.

PR 8 of the GeoTIFF test consolidation epic (#2390). Folds 13 old
reader-side test files into eight responsibility-grouped modules under
`xrspatial/geotiff/tests/read/`:

- read/test_basic.py     -- band validation on read.
- read/test_dtypes.py    -- dtype kwarg, float16 promotion, GPU dtype path.
- read/test_compression.py -- codec round-trips and decompression-bomb caps.
- read/test_tiling.py    -- per-tile / per-strip byte caps (CPU + GPU).
- read/test_endianness.py -- big-endian multi-byte GPU read path.
- read/test_nodata.py    -- GPU nodata mask helper.
- read/test_coords.py    -- descending / ascending coord round-trip.
- read/test_streaming.py -- streaming BigTIFF threshold (the one
  cross-directory move from `xrspatial/tests/`).

Tests-only restructure; no source changes. Test bodies are preserved
verbatim except for: dropped issue numbers in names, class grouping
for related cases, fixture filenames stripped of issue numbers, and
relative-import paths adjusted for the new depth (`._helpers.*` ->
`.._helpers.*`).

CLUSTER_AUDIT_PR8.md maps every old `file::test` to its new home and
gets deleted in a follow-up commit before merge.

Closes #2401. References epic #2390. PR 3 (rotated CRS) creates
`read/__init__.py` and `read/test_crs.py` in parallel; whichever PR
merges second resolves the trivial `__init__.py` conflict.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 26, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: geotiff tests: consolidate reader-path cluster

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/tests/read/test_dtypes.py:32-43, read/test_endianness.py:23-31, read/test_tiling.py:24-32 -- each module redefines a local _gpu_available() (and corresponding _HAS_GPU / _gpu_only) that already lives in xrspatial/geotiff/tests/_helpers/markers.py::gpu_available and its requires_gpu mark. The helpers are re-exported through conftest.py (see xrspatial/geotiff/tests/conftest.py:14-23) and read/test_nodata.py:11 already imports requires_gpu from there. Folding the other three modules onto the shared marker drops three near-identical helper copies and keeps the future CUDA-availability change in one place. Pre-existing pattern in the old files, but the consolidation is the natural place to absorb it.

Nits (optional improvements)

  • xrspatial/geotiff/tests/read/test_streaming.py and the codec / SOF-cap classes in read/test_compression.py exercise writer-only or codec-direct surfaces (to_geotiff, _should_use_bigtiff_streaming, _compute_classic_ifd_overhead, jpeg_decompress direct calls). The epic #2390 PR 8 directive specifically folds the streaming-BigTIFF threshold file here, so the placement is intentional, but the module docstrings should make the "this is reader-adjacent, not strictly reader" framing explicit for a future maintainer scanning read/. read/test_streaming.py already notes the epic directive at line 4; read/test_compression.py does not.

  • xrspatial/geotiff/tests/CLUSTER_AUDIT_PR8.md:182-191 ("Verification") says "12 old files deleted, 8 new files added" inside geotiff/tests/. The total deleted across the PR is 13 (the 12 inside geotiff/tests/ plus xrspatial/tests/test_geotiff_streaming_bigtiff_threshold_1785.py). The current wording reads correctly only if you treat the cross-directory move separately below it, which the text does. A one-line clarifier would remove the ambiguity, but the audit is being deleted in the merge commit anyway, so this is cosmetic.

What looks good

  • All 13 deleted files have explicit ### <old file> audit sections; every class moved out of the old modules reappears in the new file with the same class name or a clearly renamed equivalent (TestRegressionGuards -> TestFloat16RegressionGuards, TestDtypeMap -> TestFloat16DtypeMap).
  • Relative-import depth was updated correctly: read/test_tiling.py:14 uses from .._helpers.tiff_surgery import patch_byte_counts, matching the new directory depth.
  • Fixture filenames stripped of issue numbers in every renamed fixture (e.g. mb_1673.tif -> mb_band_validation.tif, forged_local_tiles_1664.tif -> forged_tiles.tif); CPU and GPU tiling fixtures coexist via a basename= parameter on _build_forged_tiled_cog, so the same tmp_path can hold both forged TIFFs without collisions.
  • Float16 GPU coverage gains a small win: the old file used a module-level pytestmark to skip the whole file when CUDA was missing, which would also skip the non-GPU TestFloat16DtypeMap unit tests. The consolidated module runs the dtype-map tests unconditionally and gates only the device-touching methods with @_gpu_only.
  • Verification numbers in the PR body match a local run: pytest xrspatial/geotiff/tests/read/ -q -> 132 passed / 2 skipped; pytest xrspatial/geotiff/tests/ -x -q -> 5716 passed; pytest xrspatial/tests/ -x -q -> 4039 passed.

Checklist

  • No source code changes (tests-only restructure).
  • All implemented backends still exercised (numpy / cupy / dask+numpy / dask+cupy) via the existing markers and @_gpu_only gates.
  • Float16 dtype-map unit tests now reachable without CUDA (regression-positive).
  • No premature materialization or new copies (no code path edits).
  • Doc cross-references updated: docs/source/reference/geotiff.rst:288-290 and docs/source/reference/release_gate_geotiff.rst:234,357,668 now point at read/test_compression.py, read/test_tiling.py, read/test_nodata.py.
  • PR 3 collision flagged in PR body; read/__init__.py is empty for trivial conflict resolution.

- Drop three local copies of the cupy availability probe in
  read/test_dtypes.py, read/test_endianness.py, and read/test_tiling.py;
  use the shared `requires_gpu` / `gpu_available` exports from
  `_helpers/markers.py` instead.
- Add a "reader-adjacent vs end-to-end-read" framing to
  read/test_compression.py so a future maintainer scanning `read/`
  understands why the codec round-trips and SOF-cap classes live there.
- Re-word the audit verification block to make the cross-directory
  move explicit (13 deleted total, -5 net across the PR).
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review after suggestion / nit fixes (0e0e3b2)

All findings from the previous review have been addressed in-PR.

Disposition

  • Suggestion 1 (GPU helper duplication) -- fixed in 0e0e3b2a. read/test_dtypes.py, read/test_endianness.py, and read/test_tiling.py now import requires_gpu / gpu_available from _helpers/markers.py via from .._helpers.markers import requires_gpu as _gpu_only (or the gpu_available callable for the tifffile-gated case). Three local _gpu_available() definitions removed.

  • Nit 1 (reader-adjacent framing in test_compression.py) -- fixed in 0e0e3b2a. The module docstring now calls out that TestDeflate, TestLZW, TestPredictor, TestDispatch, the Test*Direct codec bomb classes, and the JPEG SOF cap class call the codec functions directly rather than going through open_geotiff / read_to_array, and explains why those still live under read/ (the reader is the sole consumer of those decode entry points).

  • Nit 2 (audit verification wording) -- fixed in 0e0e3b2a. The audit's verification block now states "13 deleted total" explicitly and breaks down the net deltas by directory.

Re-verification

  • pytest xrspatial/geotiff/tests/read/ -q -> 132 passed, 2 skipped (identical pass count and skip count to the pre-fix run; the shared requires_gpu mark produces the same skip behaviour as the local helpers it replaces).

No remaining findings.

@brendancol brendancol merged commit f74eea2 into main May 26, 2026
6 of 7 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.

Consolidate reader-path tests (epic #2390 PR 8)

1 participant