Skip to content

geotiff tests: consolidate rotated-CRS cluster#2404

Merged
brendancol merged 4 commits into
mainfrom
issue-2396
May 26, 2026
Merged

geotiff tests: consolidate rotated-CRS cluster#2404
brendancol merged 4 commits into
mainfrom
issue-2396

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

Folds three rotated / dropped-CRS test files into a single parametrized file at xrspatial/geotiff/tests/read/test_crs.py under the layout from epic #2390 PR 1.

Files folded:

  • test_allow_rotated_geotiff_2115.py
  • test_allow_rotated_crs_drop_2126.py
  • test_allow_rotated_no_crs_2122.py

The HTTP rotated test (test_http_dask_allow_rotated_2130.py) is left for PR 9 (integration).

What changed

  • New read/__init__.py and read/test_crs.py with 22 parametrized tests over (scenario, chunks) where scenario is rotated_no_crs / rotated_with_crs / axis_aligned_with_crs and chunks is eager / dask.
  • GPU-eager and dask+CuPy variants kept under @requires_gpu.
  • VRT eager + chunked rotated cases parametrized over the same chunks axis.
  • Cross-file consumers of _write_rotated_tiff (test_georef_status_2136.py, test_lazy_finalization_parity_2162.py) now import from the new module.
  • Release-gate and reference docs that cite the old filenames point at read/test_crs.py.

Audit

CLUSTER_AUDIT_PR3.md maps every old file::test to its new file::test_id. Two HTTP cases from 2115 were dropped; the same coverage already exists in test_http_dask_allow_rotated_2130.py (PR 9 cluster). The audit file is deleted on a follow-up commit before approval.

Test plan

  • pytest xrspatial/geotiff/tests/read/ -v: 22 passed
  • pytest xrspatial/geotiff/tests/ -x -q: 5705 passed, 68 skipped
  • File count: 3 deleted, 1 added (net -2)

Closes #2396. Part of epic #2390.

Folds three rotated / dropped-CRS test files into a single parametrized
file at ``read/test_crs.py`` under the layout established by epic #2390
PR 1:

* ``test_allow_rotated_geotiff_2115.py``
* ``test_allow_rotated_crs_drop_2126.py``
* ``test_allow_rotated_no_crs_2122.py``

Parametrizes scenarios as ``rotated_no_crs``, ``rotated_with_crs``,
``axis_aligned_with_crs`` with ``eager`` / ``dask`` backend axes. The
GPU eager + dask+CuPy cases stay gated by ``@requires_gpu``.

The two cross-file ``_write_rotated_tiff`` consumers
(``test_georef_status_2136.py``, ``test_lazy_finalization_parity_2162.py``)
now import from ``read/test_crs.py``. The release-gate and reference
docs that cite the old filenames are updated to point at the new file.

The HTTP rotated test (``test_http_dask_allow_rotated_2130.py``) is
left in place; it belongs to PR 9 (integration cluster).

``CLUSTER_AUDIT_PR3.md`` maps every old ``file::test`` to its new
``file::test_id`` and is removed in a follow-up commit before approval.

Part of epic #2390.
@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 rotated-CRS cluster

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/geotiff/tests/read/test_crs.py (the _build_* callers around lines 357 / 374 / 390 / 410): temp filenames are built with f"..._{chunks}.tif"; when chunks is None this writes literal _None.tif paths under pytest's tmp_path. Functional but a bit ugly. Worth swapping for the test ID label ("eager" / "dask") via a small lookup.
  • xrspatial/geotiff/tests/read/test_crs.py: _build_rotated_no_crs returns (src, arr) but the default-raises tests discard arr. Trivial; src, _ = ... would make the intent obvious. Same shape in _build_axis_aligned_with_crs.
  • xrspatial/geotiff/tests/CLUSTER_AUDIT_PR3.md: the audit table lumps the two dropped HTTP cases as "covered by PR 9". Correct, but naming the specific tests in test_http_dask_allow_rotated_2130.py (e.g. test_http_dask_rotated_default_raises, test_http_dask_rotated_allow_rotated_reads) would make the trail explicit so a future reviewer does not have to open the integration file to confirm.

What looks good

  • Parametrization is the right shape: (scenario, chunks) with descriptive IDs (rotated_no_crs, rotated_with_crs, axis_aligned_with_crs crossed with eager / dask), and GPU variants stay behind @requires_gpu without bleeding the import gate into the rest of the file.
  • Both rotated TIFF writers are preserved as distinct test paths rather than collapsed: hand-rolled with the full _GEO_KEYS_4326 block, and the tifffile-based one with a Geographic-only GeoKey. Different GeoKey-parser branches; worth keeping both.
  • Unit tests for _extract_transform and _populate_attrs_from_geo_info are parametrized over the attrs they assert on (crs / crs_wkt / transform), which is cleaner than the original three-separate-functions layout.
  • Cross-file consumers of _write_rotated_tiff (test_georef_status_2136.py, test_lazy_finalization_parity_2162.py) are updated to import from the new module, with a relocation comment citing the epic PR. The two doc paths (release_gate_geotiff.rst, geotiff.rst) that cited the old filenames also point at the new location.
  • 5705 passing on the full geotiff suite locally; file count moves -2 as the epic intended.

Checklist

  • Algorithm matches reference/paper (N/A: tests-only restructure)
  • All implemented backends produce consistent results (eager / dask / GPU eager / dask+CuPy covered)
  • NaN handling is correct (N/A)
  • Edge cases are covered by tests (axis-aligned regression guard preserved)
  • Dask chunk boundaries handled correctly (chunks=2 exercised across all rotated cases)
  • No premature materialization or unnecessary copies (N/A)
  • Benchmark exists or is not needed (N/A)
  • README feature matrix updated (N/A)
  • Docstrings present and accurate (module docstring plus per-test docstrings preserved or improved)
  • CLUSTER_AUDIT_PR3.md present; will be removed in follow-up commit

Three nits from /review-pr applied:

* Add ``_chunks_label`` helper so tmp_path filenames read
  ``..._eager.tif`` / ``..._dask.tif`` instead of literal
  ``..._None.tif`` / ``..._2.tif``.
* Use ``src, _ = ...`` in the default-raises and axis-aligned tests so
  the unused fixture return is explicit.
* Spell out the specific PR 9 test names that supersede the two
  dropped HTTP cases in ``CLUSTER_AUDIT_PR3.md`` so the trail is
  explicit without opening the integration file.

All 22 ``read/`` tests still pass.
The audit is a review artifact, not a documentation deliverable. Git
history and the PR description retain the trail mapping old test
files to ``read/test_crs.py`` IDs.

Part of epic #2390.
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

All three nits from the prior review are addressed in 89d0078:

  • Filename labels: new _chunks_label helper maps None to eager and 2 to dask, so tmp_path now contains ..._eager.tif / ..._dask.tif instead of ..._None.tif. Applied across all five parametrized cases that previously interpolated the raw chunks value (the no-CRS default-raises, no-CRS optin-reads, rotated-with-CRS hand-rolled, rotated-with-CRS geokey-only, GPU, and the VRT pair).
  • Unused-binding cleanup: src, _ = _build_*(...) in the default-raises and axis-aligned tests so it is obvious the array fixture is intentionally unused at those sites.
  • Audit specifics: CLUSTER_AUDIT_PR3.md now names the two replacing PR 9 tests (test_http_dask_rotated_default_raises, test_http_dask_rotated_allow_rotated_reads) on the dropped HTTP rows.

The audit file was then removed in 283b600 before approval, per the epic plan. The 22 read/ tests still pass locally.

No new findings on the follow-up pass.

@brendancol brendancol merged commit 72dd61c into main May 26, 2026
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 rotated / dropped CRS tests (epic #2390 PR 3)

1 participant