Skip to content

geotiff: keep no-georef int coords through to_geotiff round-trip (#1949)#1954

Merged
brendancol merged 3 commits into
mainfrom
deep-sweep-metadata-geotiff-2026-05-15-1778858229-01
May 15, 2026
Merged

geotiff: keep no-georef int coords through to_geotiff round-trip (#1949)#1954
brendancol merged 3 commits into
mainfrom
deep-sweep-metadata-geotiff-2026-05-15-1778858229-01

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #1949.

Summary

Test plan

  • New unit tests on _coords_to_transform for int64 / int32 / float / 3D-yxband.
  • New round-trip tests through to_geotiff (eager numpy, dask streaming, GPU).
  • Double-round-trip stability test.
  • Explicit attrs['transform'] override test.
  • Existing test_coords_to_transform_3d_1643.py, test_metadata_round_trip_1484.py, test_attrs_parity_1548.py, test_coord_regularity_1720.py, test_coords_1813.py, test_georef_edges.py still pass.
  • Full geotiff test suite passes (9 pre-existing failures on main are unrelated to this change: test_predictor2_big_endian_gpu_1517.py, test_reader_kwarg_order_1935.py, test_size_param_validation_gpu_vrt_1776.py::TestWriteGeotiffGpuTileSize::test_tile_size_positive_works).

Copilot AI review requested due to automatic review settings May 15, 2026 15:29
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 15, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@brendancol brendancol force-pushed the deep-sweep-metadata-geotiff-2026-05-15-1778858229-01 branch from 6989b91 to 497d7c5 Compare May 15, 2026 15:46
@brendancol
Copy link
Copy Markdown
Contributor Author

PR Review: geotiff: keep no-georef int coords through to_geotiff round-trip (#1949)

Blockers (must fix before merge)

  • None

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/_coords.py:293 short-circuits before _is_regular(x, ...) runs. Integer arange coords are uniform so the regularity check would pass anyway, but if a caller ever passes hand-built int coords with non-uniform spacing (e.g. [0, 1, 3, 7]), the early return now hides that error rather than raising. A docstring note on coords_to_transform clarifying that integer dtype is treated as a no-georef sentinel (not just "uniform int coords") would head off confusion if someone later constructs int-indexed georeffed arrays.
  • xrspatial/geotiff/tests/test_no_georef_writer_round_trip_1949.py:96 iterates (np.int32, np.int16, np.uint32) inside one test function. Splitting into pytest.mark.parametrize would surface which dtype fails when one regresses, instead of stopping at the first.

Nits (optional improvements)

  • xrspatial/geotiff/tests/test_no_georef_writer_round_trip_1949.py:128 builds a 3D (y, x, band) array with int y/x coords but does not call to_geotiff on it. Extending the helper test into a round-trip on the 3D layout would close the loop with the existing 3D coverage in test_coords_to_transform_3d_1643.py.
  • xrspatial/geotiff/tests/test_no_georef_writer_round_trip_1949.py:172 uses random.default_rng(seed=1949) for round-trip pixel data but never asserts pixel-value equality after round-trip. Adding np.testing.assert_array_equal(da.data, da2.data) would catch a regression where the writer truncates or rescales pixel bytes alongside the coord fix.
  • xrspatial/geotiff/_coords.py:280 comment block is long enough that placing it inside a small helper docstring (or moving the rationale to the function docstring) would keep the function body easier to scan.

What looks good

  • The fix lives in one place (_coords.py:293) and every writer path (eager, dask, GPU, VRT) flows through it, so the no-georef contract holds across backends without duplicate guards.
  • kind in ('i', 'u') covers signed and unsigned integers at any width, matching the read-side np.int64 arange and any user-built int8/int16/uint32 variant.
  • The explicit attrs['transform'] escape hatch is exercised by test_explicit_transform_attr_still_writes, confirming the helper only intercepts the implicit-from-coords fallback.
  • Double round-trip stability test pins the fixed-point behaviour the issue was really after.
  • GPU test is gated on cupy + CUDA availability and reuses the same assertions.

Checklist

  • Algorithm matches reference/paper
  • All implemented backends produce consistent results
  • NaN handling is correct
  • Edge cases are covered by tests
  • Dask chunk boundaries handled correctly
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (not needed; metadata-only change)
  • README feature matrix updated (if applicable) (N/A)
  • Docstrings present and accurate

@brendancol brendancol force-pushed the deep-sweep-metadata-geotiff-2026-05-15-1778858229-01 branch from b3c3644 to c996afc Compare May 15, 2026 17:29
@brendancol
Copy link
Copy Markdown
Contributor Author

PR Review: geotiff: keep no-georef int coords through to_geotiff round-trip (#1949) (re-review after rebase)

Rebased onto origin/main at 9ce0e60 (Windows writer-returns fix). Conflict-free. Pushed c996afc which converts the new test module's unconditional import tifffile into pytest.importorskip("tifffile") so collection no longer aborts on CI shards that don't ship tifffile, matching the pattern in test_miniswhite_nodata_1809.py and test_predictor3_big_endian.py. Two stray unused stdlib imports (os, tempfile) were dropped at the same time. Local pytest xrspatial/geotiff/tests/test_no_georef_writer_round_trip_1949.py reports 13 passed.

Blockers

  • None

Suggestions

  • None

Nits

  • None

What looks good

Checklist

  • Rebase clean on origin/main
  • Targeted test green locally (13/13)
  • CI collection regression on ubuntu-3.12 / macos / windows addressed via importorskip
  • Force-with-lease push to origin
  • CI re-run on the new head

``_coords_to_transform`` previously inferred a unit GeoTransform from
the integer pixel coords the read-side no-georef branch emits
(``np.arange(N, dtype=np.int64)``), so writing a no-georef DataArray
back through ``to_geotiff`` silently injected ``ModelPixelScale`` /
``ModelTiepoint`` tags. On the next read the file looked
georeferenced, the coord dtype flipped to float64, and a synthetic
``attrs['transform']`` appeared.

Short-circuit when either spatial coord array has an integer dtype --
that signal only comes from the read-side no-georef fallback, so
returning ``None`` here keeps the no-georef contract across an
arbitrary number of read -> write -> read cycles on every writer path
(eager numpy, dask streaming, GPU). An explicit
``attrs['transform']`` still wins because the writers consult it
before calling this helper.

Adds ``test_no_georef_writer_round_trip_1949.py`` covering the helper
directly, eager / dask streaming / GPU round-trips, a double
round-trip, and the explicit-attr override.
tifffile is not a runtime dep, so the unconditional module-level
import broke collection on CI where tifffile isn't installed.
Switch to ``pytest.importorskip`` to skip the file gracefully
instead, matching the pattern used by other geotiff tests
(``test_miniswhite_nodata_1809``, ``test_predictor3_big_endian``).
Also drop two unused stdlib imports.
@brendancol brendancol force-pushed the deep-sweep-metadata-geotiff-2026-05-15-1778858229-01 branch from c996afc to 8da3ca7 Compare May 15, 2026 17:52
@brendancol
Copy link
Copy Markdown
Contributor Author

Rebased onto current main at 8da3ca7 to pick up the Python 3.14 CI matrix limit (b1579f8).

@brendancol brendancol merged commit a4acd22 into main May 15, 2026
4 of 5 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.

geotiff: to_geotiff turns no-georef int64 pixel coords into a fake transform on round-trip

2 participants