Skip to content

geotiff tests: consolidate GPU writer cluster (Sub-PR B)#2473

Open
brendancol wants to merge 4 commits into
mainfrom
worktree-agent-a1e860edb5f5f1256
Open

geotiff tests: consolidate GPU writer cluster (Sub-PR B)#2473
brendancol wants to merge 4 commits into
mainfrom
worktree-agent-a1e860edb5f5f1256

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes part of #2438 (Sub-PR B). Cluster 14 of long-tail epic #2424.

Summary

Fold 8 GPU writer test files into a new xrspatial/geotiff/tests/gpu/test_writer.py:

  • test_gpu_writer_attrs_1563.py (write-path attrs parity)
  • test_gpu_writer_band_first_1580.py ((band, y, x) layout remap + auto-dispatch)
  • test_gpu_writer_compression_modes_2026_05_11.py (zstd, deflate, jpeg, none)
  • test_gpu_writer_cpu_fallback_codecs_2026_05_12.py (lzw, packbits, lz4, lerc, jpeg2000)
  • test_gpu_writer_nan_sentinel_1599.py (NaN-to-sentinel rewrite)
  • test_gpu_writer_overview_inplace_1948.py (cupy.putmask in COG loop)
  • test_gpu_writer_overview_mode_and_compression_level_1740.py
  • test_to_geotiff_gpu_fallback_1674.py (gpu=True exception classification)

Tests-only. The shared requires_gpu marker from _helpers/markers.py replaces the per-file _gpu_available / _gpu_only helpers. 87 tests collected, matching the baseline sum.

The release-gate writer.gpu row and the experimental GPU codec note in docs/source/reference/geotiff.rst now cite gpu/test_writer.py.

Verification

  • pytest xrspatial/geotiff/tests/gpu/test_writer.py -v: 87 passed on a CUDA-equipped host.
  • pytest --collect-only xrspatial/geotiff/tests/gpu/test_writer.py: 87 tests collected.
  • pytest xrspatial/geotiff/tests/release_gates/test_stable_features.py: 158 passed, 1 xfailed.

Test plan

  • New consolidated gpu/test_writer.py collects and runs locally.
  • Release-gate checklist-parity test passes.

Companion PRs

One of four sub-PRs against issue #2438. Sub-PRs A (gpu/test_reader.py), C (gpu/test_codec.py), and D (gpu/test_kernels_and_kwargs.py) are independent; merge order does not matter.

Closes part of #2438 (Sub-PR B). Cluster 14 of long-tail epic #2424.

Fold 8 GPU writer test files into a new
xrspatial/geotiff/tests/gpu/test_writer.py:

- test_gpu_writer_attrs_1563.py (write-path attrs parity)
- test_gpu_writer_band_first_1580.py ((band, y, x) layout remap)
- test_gpu_writer_compression_modes_2026_05_11.py (zstd, deflate, jpeg, none)
- test_gpu_writer_cpu_fallback_codecs_2026_05_12.py (lzw, packbits, lz4, lerc, jpeg2000)
- test_gpu_writer_nan_sentinel_1599.py (NaN-to-sentinel rewrite)
- test_gpu_writer_overview_inplace_1948.py (cupy.putmask in COG loop)
- test_gpu_writer_overview_mode_and_compression_level_1740.py
- test_to_geotiff_gpu_fallback_1674.py (gpu=True exception classification)

Tests-only. The shared requires_gpu marker from _helpers/markers.py
replaces the per-file _gpu_available / _gpu_only helpers. 87 tests
collected, matching the baseline sum across the eight source files.

The release-gate row for writer.gpu and the geotiff.rst experimental
codec note now cite gpu/test_writer.py.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 27, 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 GPU writer cluster (Sub-PR B)

Blockers

None.

Suggestions

None.

Nits

  • xrspatial/geotiff/tests/gpu/test_writer.py:37 -- importlib.util is imported at module scope but the only remaining caller is _nvjpeg_available. The original per-file _gpu_available helpers also used it; here the GPU detection delegates to _helpers/markers.py, so it only feeds the nvjpeg probe. Could be moved inside _nvjpeg_available, but a stdlib import at module scope is the more conventional choice and I would leave it.
  • xrspatial/geotiff/tests/gpu/test_writer.py:1398 -- the source-locate path in test_gpu_writer_overview_loop_uses_putmask_1948 now walks parent.parent.parent / "_writers" / "gpu.py" (the original used parent.parent). The audit file calls this out, but a one-line comment in the test docstring would put the explanation where the next reader actually lands. Optional.

What looks good

  • Every test name from the eight source files is present in the new file. 68 unique functions, 87 parametrise-expanded cases, matching the pre-consolidation baseline.
  • _make_int_da (Section 3, 64x64 int32) and _make_int_da_small (Section 4, 32x32 uint16) are deliberately kept distinct so the test inputs each section saw stay the same. Merging the two helpers would have changed inputs.
  • Per-file _gpu_available / _gpu_only helpers are replaced with the shared requires_gpu marker from _helpers/markers.py, matching the convention from #2470. The fallback-section tests keep no @_gpu_only decorator on purpose (they monkeypatch write_geotiff_gpu), as they did originally.
  • Release-gate citations in docs/source/reference/release_gate_geotiff.rst and docs/source/reference/geotiff.rst point at the new file. pytest xrspatial/geotiff/tests/release_gates/: 158 passed, 1 xfailed locally.
  • CLUSTER_AUDIT_GPU_B.md maps every old test to its new location and will be deleted in the final pre-merge commit per the epic's hard gate.

Checklist

  • All original test names migrated
  • Tests-only PR as scoped (no source changes)
  • requires_gpu marker correctly applied
  • Path resolution in source-locate test verified (walks to _writers/gpu.py)
  • Helper defaults preserved per source file
  • Release-gate citation updated; release-gate parity test passes
  • Audit file created for the epic gate

Address review nit: the overview-loop source-locate test walks
parent.parent.parent (not the original parent.parent) because the
consolidated file lives one level deeper under tests/gpu/. Make
that explicit in the docstring so future readers do not assume the
walk is wrong.
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

Picked up at commit 1e286ff.

Blockers

None.

Suggestions

None.

Nits

None remaining. The earlier nit on the source-locate path docstring was addressed in 1e286ff with a short path note. The importlib.util module-scope import was kept (conventional placement for a stdlib import; the helper that uses it is defined later in the file).

Re-run

  • pytest xrspatial/geotiff/tests/gpu/test_writer.py: 87 passed.
  • pytest xrspatial/geotiff/tests/release_gates/: 158 passed, 1 xfailed.

Ready as-is from the review side. Merging gated only on CI and the final pre-merge audit-file deletion per the epic #2424 hard gate.

Final pre-merge commit per the epic #2424 hard gate: the audit file
that maps every old file::test to its new file::test_id is deleted
before merge. The audit served the review and is now baked into the
PR diff itself.
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.

1 participant