geotiff: deprecate geographic-CRS GeoKey attrs in DataArray.attrs (#1984)

The six geographic-CRS GeoKey-derived attrs (crs_name, geog_citation,
datum_code, angular_units, semi_major_axis, inv_flattening) were
documented in the attrs contract as best-effort pass-through, but the
PR6 locking test (#2004) showed they never round-trip: build_geo_tags
only emits the primary GEOKEY_GEOGRAPHIC_TYPE, so the secondary
GeoKeys these attrs come from are never written back.

PR 7 of the issue #1984 plan deprecates the read-side emission. Each
emission now fires a DeprecationWarning with a stable wording. The
values still land in attrs for one release cycle so external callers
can migrate; removal is scheduled for a later release.

Changes:

- _attrs.py: add _DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS tuple,
  _deprecated_geographic_geokey_warning text helper, and
  _emit_deprecated_geographic_geokey emission helper. Route the six
  attrs through the helper in _populate_attrs_from_geo_info.
- _attrs.py docstring: move the six attrs out of the best-effort
  pass-through tier and into a new "Deprecated" tier section.
- New test_attrs_pr7_deprecate_geographic_1984.py with parametrised
  warning assertions, still-emits-during-deprecation assertions, a
  no-warning-when-field-absent guard, and a warning-text shape check.
- CHANGELOG.md: document the deprecation under Unreleased.

Author: brendancol

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Unreleased
 
 #### Bug fixes and improvements
+- Deprecate read-side emission of geographic-CRS GeoKey attrs (crs_name, geog_citation, datum_code, angular_units, semi_major_axis, inv_flattening); the writer cannot reconstruct them so they do not round-trip. These attrs still emit for now but trigger a DeprecationWarning. Removal planned for a future release. (#1984)
 - Refresh the geotiff mmap cache when a file is replaced under the same path so re-reads after an atomic-rename overwrite no longer return stale bytes
 - Decode TIFF predictor=3 un-transpose by file byte order so big-endian floating-point TIFFs read back exactly
 - Default internal `_vrt.read_vrt` `missing_sources` to `'raise'` so an unreadable VRT source no longer produces a silent zero-fill hole on integer rasters; pass `missing_sources='warn'` to opt back into the previous lenient behaviour (#1843)

xrspatial/geotiff/_attrs.py

@@ -61,13 +61,7 @@
 Best-effort pass-through (preserved when the writer can reconstruct
 from canonical state, otherwise dropped on round-trip):
 
-- ``crs_name``: human-readable CRS name from the GeoKey directory.
-- ``geog_citation``: GeographicTypeGeoKey citation string.
-- ``datum_code``: GeogGeodeticDatumGeoKey value.
-- ``angular_units``: GeogAngularUnitsGeoKey value.
 - ``linear_units``: ProjLinearUnitsGeoKey value.
-- ``semi_major_axis``: GeogSemiMajorAxisGeoKey value.
-- ``inv_flattening``: GeogInvFlatteningGeoKey value.
 - ``projection_code``: ProjectedCSTypeGeoKey value.
 - ``vertical_crs``: VerticalCSTypeGeoKey value.
 - ``vertical_citation``: VerticalCitationGeoKey value.
@@ -76,6 +70,22 @@
 - ``extra_samples``: TIFF ExtraSamples tag.
 - ``colormap``, ``colormap_rgba``, ``cmap``: palette data attached to
   single-band paletted images.
+
+Deprecated (will be removed in a future release; see issue #1984):
+
+These attrs are still emitted on read for one release cycle, but each
+emission triggers a ``DeprecationWarning``. The writer's
+``build_geo_tags`` only emits the primary geographic GeoKey, so the
+secondary GeoKeys these attrs are derived from are never written and
+the values do not survive a write -> read round-trip. Callers should
+stop relying on them and use ``crs`` / ``crs_wkt`` instead.
+
+- ``crs_name``: human-readable CRS name from the GeoKey directory.
+- ``geog_citation``: GeographicTypeGeoKey citation string.
+- ``datum_code``: GeogGeodeticDatumGeoKey value.
+- ``angular_units``: GeogAngularUnitsGeoKey value.
+- ``semi_major_axis``: GeogSemiMajorAxisGeoKey value.
+- ``inv_flattening``: GeogInvFlatteningGeoKey value.
 """
 from __future__ import annotations
 
@@ -125,6 +135,52 @@
 _RESOLUTION_UNIT_IDS = {'none': 1, 'inch': 2, 'centimeter': 3}
 
 
+# Geographic-CRS GeoKey-derived attrs scheduled for removal (issue #1984
+# PR 7). The writer's ``build_geo_tags`` only emits the primary
+# GEOKEY_GEOGRAPHIC_TYPE, never the secondary geographic GeoKeys these
+# attrs are derived from. The values therefore never round-trip. Keep
+# emitting them for one release cycle so external callers can migrate,
+# then drop the emission entirely.
+_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS = (
+    'crs_name',
+    'geog_citation',
+    'datum_code',
+    'angular_units',
+    'semi_major_axis',
+    'inv_flattening',
+)
+
+
+def _deprecated_geographic_geokey_warning(name: str) -> str:
+    """Warning text shared by every deprecated geographic-GeoKey attr.
+
+    Centralised so the test suite can match the wording verbatim and so
+    every emission site uses identical phrasing. See issue #1984 PR 7.
+    """
+    return (
+        f"xrspatial.geotiff: attrs[{name!r}] is deprecated; the writer "
+        f"cannot reconstruct it from the canonical CRS so it will not "
+        f"round-trip. It will be removed in a future release. See "
+        f"issue #1984."
+    )
+
+
+def _emit_deprecated_geographic_geokey(attrs: dict, name: str, value) -> None:
+    """Emit a deprecated geographic-GeoKey attr with a DeprecationWarning.
+
+    Sets ``attrs[name] = value`` (keeping the read-side emission alive
+    for one release cycle) after firing a ``DeprecationWarning`` so
+    callers learn the attr is going away. Used for the six attrs listed
+    in ``_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS``.
+    """
+    warnings.warn(
+        _deprecated_geographic_geokey_warning(name),
+        DeprecationWarning,
+        stacklevel=2,
+    )
+    attrs[name] = value
+
+
 def _extent_to_window(transform, file_height, file_width,
                       y_min, y_max, x_min, x_max):
     """Convert geographic extent to pixel window (row_start, col_start, row_stop, col_stop).
@@ -240,19 +296,24 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None
         )
 
     if geo_info.crs_name is not None:
-        attrs['crs_name'] = geo_info.crs_name
+        _emit_deprecated_geographic_geokey(attrs, 'crs_name', geo_info.crs_name)
     if geo_info.geog_citation is not None:
-        attrs['geog_citation'] = geo_info.geog_citation
+        _emit_deprecated_geographic_geokey(
+            attrs, 'geog_citation', geo_info.geog_citation)
     if geo_info.datum_code is not None:
-        attrs['datum_code'] = geo_info.datum_code
+        _emit_deprecated_geographic_geokey(
+            attrs, 'datum_code', geo_info.datum_code)
     if geo_info.angular_units is not None:
-        attrs['angular_units'] = geo_info.angular_units
+        _emit_deprecated_geographic_geokey(
+            attrs, 'angular_units', geo_info.angular_units)
     if geo_info.linear_units is not None:
         attrs['linear_units'] = geo_info.linear_units
     if geo_info.semi_major_axis is not None:
-        attrs['semi_major_axis'] = geo_info.semi_major_axis
+        _emit_deprecated_geographic_geokey(
+            attrs, 'semi_major_axis', geo_info.semi_major_axis)
     if geo_info.inv_flattening is not None:
-        attrs['inv_flattening'] = geo_info.inv_flattening
+        _emit_deprecated_geographic_geokey(
+            attrs, 'inv_flattening', geo_info.inv_flattening)
     if geo_info.projection_code is not None:
         attrs['projection_code'] = geo_info.projection_code
     if geo_info.vertical_epsg is not None:

xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py

@@ -0,0 +1,164 @@
+"""Deprecation-warning tests for the geographic-CRS GeoKey attrs.
+
+Issue #1984, PR 7 of 7.
+
+The six geographic-CRS GeoKey-derived attrs listed below were
+documented in the contract as best-effort pass-through, but the
+locking test in ``test_attrs_contract_passthrough_1984.py`` (issue
+#1984, PR 6, merged as #2004) showed they never round-trip: the
+writer's ``build_geo_tags`` only emits the primary
+``GEOKEY_GEOGRAPHIC_TYPE`` plus citation, so the secondary GeoKeys
+these attrs come from are never written.
+
+PR 7 keeps emitting the attrs on read for one release cycle so
+callers can migrate, but each emission now fires a
+``DeprecationWarning``. This file pins that warning behaviour:
+
+* One ``test_warns_<attr>`` per attr asserts a ``DeprecationWarning``
+  with the canonical wording fires when ``_populate_attrs_from_geo_info``
+  sees the matching ``GeoInfo`` field set.
+* ``test_emission_still_present`` asserts the attr value still lands
+  in ``attrs`` (i.e. PR 7 is warning-only; removal is a later PR).
+
+The test drives ``_populate_attrs_from_geo_info`` directly with a
+synthetic :class:`GeoInfo`. That bypasses ``open_geotiff`` and the
+writer, both of which are irrelevant here: the contract change is on
+the read-side attrs population step, not on the on-disk GeoKey set.
+"""
+from __future__ import annotations
+
+import warnings
+
+import pytest
+
+from xrspatial.geotiff._attrs import (
+    _DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS,
+    _deprecated_geographic_geokey_warning,
+    _populate_attrs_from_geo_info,
+)
+from xrspatial.geotiff._geotags import GeoInfo
+
+
+# (attr_name, GeoInfo field name, sample value).
+#
+# Sample values mirror what the GeoTIFF spec would put in each
+# secondary GeoKey for WGS 84 (EPSG:4326). The exact values do not
+# matter for the warning assertion, but using realistic ones keeps the
+# test useful as documentation.
+_DEPRECATED_CASES = [
+    ('crs_name',        'crs_name',        'WGS 84'),
+    ('geog_citation',   'geog_citation',   'WGS 84'),
+    ('datum_code',      'datum_code',      6326),
+    ('angular_units',   'angular_units',   'degree'),
+    ('semi_major_axis', 'semi_major_axis', 6378137.0),
+    ('inv_flattening',  'inv_flattening',  298.257223563),
+]
+
+
+def _geo_info_with(**fields) -> GeoInfo:
+    """Build a minimal :class:`GeoInfo` with only the given fields set.
+
+    A bare ``GeoInfo()`` has every optional field at ``None`` so the
+    other emission branches in ``_populate_attrs_from_geo_info`` stay
+    quiet. Only the field under test is populated, which keeps the
+    warning under test the only ``DeprecationWarning`` raised.
+    """
+    info = GeoInfo()
+    for name, value in fields.items():
+        setattr(info, name, value)
+    return info
+
+
+def test_deprecated_cases_cover_all_attrs():
+    """The parametrised case list must enumerate every attr listed in
+    ``_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS``. A drift here would silently
+    drop coverage for whichever attr was forgotten."""
+    case_attrs = {c[0] for c in _DEPRECATED_CASES}
+    assert case_attrs == set(_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS), (
+        f"deprecated cases drift from the module-level tuple:\n"
+        f"  only in cases : {sorted(case_attrs - set(_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS))}\n"
+        f"  only in module: {sorted(set(_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS) - case_attrs)}"
+    )
+
+
+@pytest.mark.parametrize(
+    'attr,field,value',
+    _DEPRECATED_CASES,
+    ids=[c[0] for c in _DEPRECATED_CASES],
+)
+def test_warns_on_emission(attr, field, value):
+    """Each deprecated geographic-GeoKey attr fires a DeprecationWarning
+    with the canonical wording when ``_populate_attrs_from_geo_info``
+    emits it."""
+    info = _geo_info_with(**{field: value})
+
+    with warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter('always')
+        attrs: dict = {}
+        _populate_attrs_from_geo_info(attrs, info)
+
+    matching = [
+        w for w in caught
+        if issubclass(w.category, DeprecationWarning)
+        and _deprecated_geographic_geokey_warning(attr) == str(w.message)
+    ]
+    assert len(matching) == 1, (
+        f"expected exactly one DeprecationWarning for {attr!r}; got "
+        f"{[(w.category.__name__, str(w.message)) for w in caught]}"
+    )
+
+
+@pytest.mark.parametrize(
+    'attr,field,value',
+    _DEPRECATED_CASES,
+    ids=[c[0] for c in _DEPRECATED_CASES],
+)
+def test_emission_still_present(attr, field, value):
+    """Deprecation-period contract: the attr value still lands in attrs.
+
+    Removal is a later PR. If a reader change drops the emission
+    entirely while this test still expects presence, the failure here
+    is the signal to bump the contract version and move the attr from
+    the deprecated tier to the removed tier in the docstring."""
+    info = _geo_info_with(**{field: value})
+
+    with warnings.catch_warnings():
+        warnings.simplefilter('ignore', DeprecationWarning)
+        attrs: dict = {}
+        _populate_attrs_from_geo_info(attrs, info)
+
+    assert attr in attrs, (
+        f"deprecated attr {attr!r} was dropped during PR 7's warning-only "
+        f"phase. PR 7 keeps emitting; removal is scheduled for a later "
+        f"release. attrs keys present: {sorted(attrs.keys())}"
+    )
+    assert attrs[attr] == value
+
+
+def test_no_warning_when_field_absent():
+    """A GeoInfo with none of the deprecated fields set fires no
+    DeprecationWarning. Guards against an unconditional warning that
+    would spam every read of every TIFF."""
+    info = GeoInfo()
+
+    with warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter('always')
+        attrs: dict = {}
+        _populate_attrs_from_geo_info(attrs, info)
+
+    dep = [w for w in caught if issubclass(w.category, DeprecationWarning)]
+    assert dep == [], (
+        f"DeprecationWarning fired even though no deprecated field was "
+        f"set on the GeoInfo: {[str(w.message) for w in dep]}"
+    )
+
+
+def test_warning_message_format():
+    """Sanity-check the warning text shape so the canonical wording
+    stays stable across the deprecation cycle."""
+    msg = _deprecated_geographic_geokey_warning('crs_name')
+    assert "xrspatial.geotiff" in msg
+    assert "attrs['crs_name']" in msg
+    assert "deprecated" in msg
+    assert "round-trip" in msg
+    assert "#1984" in msg