Skip to content

geotiff: gate non-uniform-coord check on no-georef marker, drop dtype exemption (#2133)#2143

Merged
brendancol merged 3 commits into
mainfrom
issue-2133
May 19, 2026
Merged

geotiff: gate non-uniform-coord check on no-georef marker, drop dtype exemption (#2133)#2143
brendancol merged 3 commits into
mainfrom
issue-2133

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Threads no_georef_marker through the write-metadata validation
    context from the three writer entry points (to_geotiff eager CPU,
    GPU write_geotiff_gpu, dask-VRT streaming).
  • _check_write_non_uniform_coords now gates the exemption on the
    marker instead of dtype.kind. A user-authored int-coord grid with
    non-uniform spacing raises NonUniformCoordsError; a marker-stamped
    placeholder grid still skips the check.
  • Drops _is_no_georef_sentinel from _coords.py. The predicate was
    test-only after geotiff: to_geotiff silently strips georef on int64 step-1 user coords #2120; the shape check moves into
    test_int_coord_sentinel_2087.py as a local helper.

Backend coverage

numpy, cupy, dask+numpy, dask+cupy. All four write paths pass the
marker into the validator.

Test plan

  • pytest xrspatial/geotiff/tests/ (CPU paths) passes locally
  • New test_no_georef_attr_migration_2133.py covers the validator
    gap, marker presence on every read backend, marker survival
    across da.copy() / da.assign_attrs(...), and a 3D round-trip
  • test_int_coord_sentinel_2087.py::test_non_uniform_int_coords_raise
    still passes (regex relaxed because the validator now catches the
    case before coords_to_transform)
  • GPU read-marker tests skip on this box (no CUDA); CI runs them

Closes #2133.

…#2133)

The write-side ``_check_write_non_uniform_coords`` validator skipped
its uniformity check whenever a coord was integer-dtype, on the
assumption that integer coords were the reader's ``0..N-1``
placeholder. After #2087 / #2120 that assumption no longer holds:
a user-authored int-coord grid is now a normal projected grid and
must be validated.

This change reads ``attrs[_NO_GEOREF_KEY]`` (threaded through the
validation context from all three writer entry points) instead of
peeking at ``dtype.kind``, so non-uniform int-coord grids raise
``NonUniformCoordsError`` and marker-stamped placeholder grids
still skip the check.

Also removes ``_is_no_georef_sentinel`` from ``_coords.py`` (the
helper was diagnostic-only after #2120 and only the
``test_int_coord_sentinel_2087.py`` test pinned the predicate; the
predicate moves into the test file as a local helper for the
on-disk shape assertions).

New tests cover:
- non-uniform int coords without the marker now raise via the validator
- the marker survives ``da.copy()`` / ``da.assign_attrs(...)``
- the marker is present on read across CPU eager / CPU dask / GPU
  eager / GPU dask
- 3D ``(band, y, x)`` no-georef round-trips preserve the marker

Closes #2133.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 19, 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: gate non-uniform-coord check on no-georef marker, drop dtype exemption (#2133)

Blockers

None.

Suggestions

  • docs/source/user_guide/attrs_contract.rst:107-116: the
    _xrspatial_no_georef entry covers round-trip semantics but skips
    the identity-check invariant (is True only). The issue body
    flagged this as worth pinning so a third-party stamp like
    '_xrspatial_no_georef': 'yes' can't flip writer behaviour by
    accident. One line would close that gap.

Nits

  • xrspatial/geotiff/_coords.py:41: _has_no_georef_marker(da) lost
    its xr.DataArray annotation when it became tolerant of raw
    numpy / cupy arrays. The docstring covers the new contract, but
    an Any annotation would keep the signature self-documenting.
  • xrspatial/geotiff/_validation.py:765-768: the dtype guard now
    accepts both np.floating and np.integer. np.bool_ is a
    subdtype of integer in numpy, so a bool coord array would flow
    through the diff math. Bool coords would be very unusual; mention
    rather than fix.

What looks good

  • Marker is threaded through all three writer entry points
    (to_geotiff, _write_vrt_tiled, write_geotiff_gpu) the same way.
  • Validator exemption now matches the writer's _has_no_georef_marker
    call, so the two stay in sync.
  • _has_no_georef_marker handles raw numpy / cupy arrays passed
    straight to write_geotiff_gpu. The GPU test suite caught the
    earlier AttributeError and the fallback was added before push.
  • Sentinel helper moved into the test file as a local predicate.
    Production code no longer carries dead diagnostic code.
  • New test file covers the four acceptance criteria from the issue:
    validator gates on marker, marker present on every read backend,
    marker survives copy() / assign_attrs(), 3D round-trip.
  • Existing test_non_uniform_int_coords_raise regex relaxed
    correctly. The validator now catches the case before
    coords_to_transform does, so the new message is the authoritative
    one.

Checklist

  • Algorithm matches the #2133 acceptance criteria
  • All write backends thread the marker consistently
  • NaN handling: not applicable for this change
  • Edge cases covered by tests (3D, copy, assign_attrs, marker exemption)
  • Dask boundaries: not applicable (metadata-only change)
  • No premature materialization
  • Benchmark not needed for a validator change
  • README feature matrix: not applicable (no new public API)
  • Docstrings present and updated

… predicate (#2133)

- attrs_contract.rst: pin the ``is True`` identity-check semantics
  for ``_xrspatial_no_georef`` so a third-party stamp like
  ``'yes'`` cannot silently flip the writer into no-georef mode.
- _coords.py: restore an ``Any`` annotation on
  ``_has_no_georef_marker`` now that it accepts non-DataArray inputs
  (raw ``numpy.ndarray`` / ``cupy.ndarray`` passed straight to
  ``write_geotiff_gpu``). Keeps the signature self-documenting.

The third review nit (bool dtype falling through the dtype guard)
was a false alarm: ``np.issubdtype(np.bool_, np.integer)`` is
``False`` in numpy, so bool coord arrays are already rejected.
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 dispositions

Suggestion (fixed)

  • docs/source/user_guide/attrs_contract.rst: added the identity-check invariant note. The entry now spells out that only the exact boolean True flips the writer into no-georef mode, with examples of stamps the writer ignores. Closes the gap from the original review.

Nit (fixed)

  • xrspatial/geotiff/_coords.py:_has_no_georef_marker: restored a type annotation. da: Any makes the predicate's tolerance of non-DataArray inputs explicit in the signature, not just the docstring.

Nit (dismissed)

  • xrspatial/geotiff/_validation.py:765-768 bool-coord concern: false alarm. np.issubdtype(np.bool_, np.integer) returns False in numpy, so the existing dtype guard already rejects bool coord arrays. No code change needed. The follow-up commit message records the reason.

CPU-side test suites (test_no_georef_attr_migration_2133.py, test_int_coord_sentinel_2087.py, test_no_georef_marker_2120.py, test_ambiguous_metadata_hooks_1987.py) re-run green after the follow-up commit.

@brendancol brendancol merged commit 5fc38b3 into main May 19, 2026
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: finish migration from integer coord sentinel to explicit no-georef attr

1 participant