From c526d4743c6fdc6a98c090291aba3c14d7bd1b34 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 20:41:21 -0700 Subject: [PATCH 1/3] geotiff: deprecate matplotlib colormap variants in DataArray.attrs (#1984) Issue #1984 PR 7. The reader emits ``attrs['cmap']`` (when matplotlib is importable) and ``attrs['colormap_rgba']`` whenever the source file declares Photometric=3 (palette). The writer never selects Photometric=3 from attrs alone, so PR 6 (#2004) locked the round-trip drop for both keys as part of the attrs-contract pass-through tier. This PR deprecates the read-side emission of both keys for one release cycle. Callers who want a matplotlib ListedColormap should build one from the canonical ``attrs['colormap']`` (raw uint16 RGB triples from TIFF tag 320) instead. ``attrs['colormap']`` is the canonical-tier replacement and is intentionally kept: it round-trips through ``_merge_friendly_extra_tags`` already. During the deprecation window both attrs still land on the returned DataArray; only a DeprecationWarning is added. Removal is a follow-up PR. The new attrs-contract docstring split moves both keys into a "Deprecated" section so the contract stays explicit about which keys have a future. The new test file ``xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py`` pins three behaviours: the warning fires on a Photometric=3 fixture for ``colormap_rgba`` (always) and for ``cmap`` (when matplotlib is installed); the attrs are still emitted; the plain ``attrs['colormap']`` key is unaffected. Existing tests that read palette fixtures (the TestPalette block in ``test_features.py`` and ``test_colormap_round_trip`` in ``test_metadata_round_trip_1484.py``) ignore the new DeprecationWarning locally to keep their reports clean. --- CHANGELOG.md | 1 + xrspatial/geotiff/_attrs.py | 42 ++- ...rs_pr7_deprecate_colormap_variants_1984.py | 272 ++++++++++++++++++ xrspatial/geotiff/tests/test_features.py | 7 + .../tests/test_metadata_round_trip_1484.py | 11 +- 5 files changed, 330 insertions(+), 3 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ae2b872d..7076634d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - 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) +- Deprecate read-side emission of matplotlib colormap-derived attrs (cmap, colormap_rgba) on palette TIFFs; the writer cannot set Photometric=3 so they do not round-trip. Construct ListedColormap from attrs['colormap'] in caller code. These attrs still emit for now but trigger a DeprecationWarning. Removal planned for a future release. (#1984) ### Version 0.9.9 - 2026-05-05 diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index 865138ae7..597e66be9 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -74,8 +74,19 @@ - ``vertical_units``: VerticalUnitsGeoKey value. - ``image_description``: TIFF ImageDescription tag. - ``extra_samples``: TIFF ExtraSamples tag. -- ``colormap``, ``colormap_rgba``, ``cmap``: palette data attached to - single-band paletted images. +- ``colormap``: raw uint16 RGB triples from the TIFF ColorMap tag (320), + attached to single-band paletted images. + +Deprecated (will be removed in a future release; see issue #1984): + +- ``colormap_rgba``: RGBA palette array, only emitted on read when the + source file is Photometric==3 (palette). The writer never selects + Photometric=3, so this attr does not round-trip. Construct an RGBA + palette from ``attrs['colormap']`` in caller code if needed. +- ``cmap``: matplotlib ``ListedColormap`` built from the palette. Same + Photometric==3 gate, same round-trip gap. Construct a + ``ListedColormap`` from ``attrs['colormap']`` in caller code if + needed. """ from __future__ import annotations @@ -286,10 +297,37 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None if geo_info.colormap is not None: try: from matplotlib.colors import ListedColormap + warnings.warn( + "xrspatial.geotiff: attrs['cmap'] is deprecated; the writer " + "cannot set Photometric=3 so it does not round-trip. " + "Construct a ListedColormap from attrs['colormap'] in caller " + "code if needed. It will be removed in a future release. " + "See issue #1984.", + DeprecationWarning, + stacklevel=2, + ) attrs['cmap'] = ListedColormap( geo_info.colormap, name='tiff_palette') + warnings.warn( + "xrspatial.geotiff: attrs['colormap_rgba'] is deprecated; " + "the writer cannot set Photometric=3 so it does not " + "round-trip. Construct a ListedColormap from " + "attrs['colormap'] in caller code if needed. It will be " + "removed in a future release. See issue #1984.", + DeprecationWarning, + stacklevel=2, + ) attrs['colormap_rgba'] = geo_info.colormap except ImportError: + warnings.warn( + "xrspatial.geotiff: attrs['colormap_rgba'] is deprecated; " + "the writer cannot set Photometric=3 so it does not " + "round-trip. Construct a ListedColormap from " + "attrs['colormap'] in caller code if needed. It will be " + "removed in a future release. See issue #1984.", + DeprecationWarning, + stacklevel=2, + ) attrs['colormap_rgba'] = geo_info.colormap if geo_info.extra_tags is not None: diff --git a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py new file mode 100644 index 000000000..e8502c2f8 --- /dev/null +++ b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py @@ -0,0 +1,272 @@ +"""Deprecation locking test for the matplotlib colormap-derived attrs. + +Issue #1984, PR 7. The reader emits ``attrs['cmap']`` (when matplotlib +is importable) and ``attrs['colormap_rgba']`` whenever the source file +declares ``Photometric=3`` (palette). The writer never selects +``Photometric=3`` from attrs alone, so a write -> read cycle drops both +keys. PR 6 (#2004) locked that drop in the pass-through contract. + +PR 7 deprecates the read-side emission for one release cycle: callers +who want a matplotlib ``ListedColormap`` should build one from the +canonical ``attrs['colormap']`` (raw uint16 RGB triples from TIFF tag +320) instead. The plain ``attrs['colormap']`` is the canonical tier +attr and still round-trips through ``_merge_friendly_extra_tags``; +this PR does not touch it. + +This file pins three behaviours: + +1. Reading a ``Photometric=3`` TIFF emits ``DeprecationWarning`` for + ``colormap_rgba`` (always) and for ``cmap`` (only when matplotlib + is importable). +2. Both attrs still ARE emitted on the resulting DataArray during the + deprecation window. Removing emission would break callers who have + not yet migrated; that removal is a follow-up PR. +3. The plain ``attrs['colormap']`` is unaffected: no warning fires on + it, and it still lands in attrs. +""" +from __future__ import annotations + +import importlib.util +import struct +import warnings + +import numpy as np +import pytest + +from xrspatial.geotiff import open_geotiff + + +_HAS_MATPLOTLIB = importlib.util.find_spec("matplotlib") is not None + + +def _make_palette_uint8_tiff(path, pixels, palette_rgb16): + """Write an 8-bit, 256-entry palette TIFF (Photometric=3) directly. + + Mirrors the helper in ``test_metadata_round_trip_1484.py``. The + writer in xrspatial cannot emit Photometric=3, so the deprecation + path is only reachable via a hand-built fixture like this one. + """ + bo = '<' + width = pixels.shape[1] + height = pixels.shape[0] + n_colors = 256 + assert len(palette_rgb16) == n_colors + + flat = pixels.ravel().astype(np.uint8) + pixel_bytes = flat.tobytes() + + r_vals = [c[0] for c in palette_rgb16] + g_vals = [c[1] for c in palette_rgb16] + b_vals = [c[2] for c in palette_rgb16] + cmap_values = r_vals + g_vals + b_vals + + tag_list = [] + + def add_short(tag, val): + tag_list.append((tag, 3, 1, struct.pack(f'{bo}H', val))) + + def add_long(tag, val): + tag_list.append((tag, 4, 1, struct.pack(f'{bo}I', val))) + + def add_shorts(tag, vals): + tag_list.append( + (tag, 3, len(vals), + struct.pack(f'{bo}{len(vals)}H', *vals))) + + add_short(256, width) + add_short(257, height) + add_short(258, 8) # bits per sample + add_short(259, 1) # no compression + add_short(262, 3) # photometric = palette + add_short(277, 1) # samples per pixel = 1 + add_short(278, height) # rows per strip + add_long(273, 0) # strip offsets placeholder + add_long(279, len(pixel_bytes)) + add_shorts(320, cmap_values) # ColorMap + add_short(339, 1) # sample format = uint + + tag_list.sort(key=lambda t: t[0]) + num_entries = len(tag_list) + ifd_start = 8 + ifd_size = 2 + 12 * num_entries + 4 + overflow_start = ifd_start + ifd_size + + overflow_buf = bytearray() + tag_offsets = {} + for tag, _typ, _count, raw in tag_list: + if len(raw) > 4: + tag_offsets[tag] = len(overflow_buf) + overflow_buf.extend(raw) + if len(overflow_buf) % 2: + overflow_buf.append(0) + else: + tag_offsets[tag] = None + + pixel_data_start = overflow_start + len(overflow_buf) + + patched = [] + for tag, typ, count, raw in tag_list: + if tag == 273: + patched.append((tag, typ, count, + struct.pack(f'{bo}I', pixel_data_start))) + else: + patched.append((tag, typ, count, raw)) + tag_list = patched + + overflow_buf = bytearray() + tag_offsets = {} + for tag, _typ, _count, raw in tag_list: + if len(raw) > 4: + tag_offsets[tag] = len(overflow_buf) + overflow_buf.extend(raw) + if len(overflow_buf) % 2: + overflow_buf.append(0) + else: + tag_offsets[tag] = None + + out = bytearray() + out.extend(b'II') + out.extend(struct.pack(f'{bo}H', 42)) + out.extend(struct.pack(f'{bo}I', ifd_start)) + out.extend(struct.pack(f'{bo}H', num_entries)) + for tag, typ, count, raw in tag_list: + out.extend(struct.pack(f'{bo}HHI', tag, typ, count)) + if len(raw) <= 4: + out.extend(raw.ljust(4, b'\x00')) + else: + ptr = overflow_start + tag_offsets[tag] + out.extend(struct.pack(f'{bo}I', ptr)) + out.extend(struct.pack(f'{bo}I', 0)) + out.extend(overflow_buf) + out.extend(pixel_bytes) + + with open(path, 'wb') as f: + f.write(bytes(out)) + + +def _palette_fixture(tmp_path, name='palette_pr7_1984.tif'): + """Build a 2x5 uint8 palette TIFF with a 256-entry RGB palette.""" + palette = [(i * 257, (255 - i) * 257, (i * 2) % 65536) + for i in range(256)] + pixels = np.array([[0, 1, 2, 254, 255], + [10, 20, 30, 40, 50]], dtype=np.uint8) + path = str(tmp_path / name) + _make_palette_uint8_tiff(path, pixels, palette) + return path + + +def test_colormap_rgba_emits_deprecation_warning(tmp_path): + """Reading a Photometric=3 TIFF triggers DeprecationWarning for + ``attrs['colormap_rgba']``. + + Fires regardless of whether matplotlib is installed: the reader + sets ``colormap_rgba`` on both the matplotlib branch and the + ImportError fallback branch. + """ + path = _palette_fixture(tmp_path, name='palette_rgba_warn.tif') + + with pytest.warns(DeprecationWarning) as record: + open_geotiff(path) + + matched = [ + w for w in record + if "attrs['colormap_rgba']" in str(w.message) + and 'issue #1984' in str(w.message) + ] + assert matched, ( + "Expected a DeprecationWarning mentioning attrs['colormap_rgba'] " + "and issue #1984; got: " + f"{[str(w.message) for w in record]}" + ) + + +@pytest.mark.skipif( + not _HAS_MATPLOTLIB, reason="matplotlib not installed" +) +def test_cmap_emits_deprecation_warning(tmp_path): + """Reading a Photometric=3 TIFF triggers DeprecationWarning for + ``attrs['cmap']`` when matplotlib is installed.""" + path = _palette_fixture(tmp_path, name='palette_cmap_warn.tif') + + with pytest.warns(DeprecationWarning) as record: + open_geotiff(path) + + matched = [ + w for w in record + if "attrs['cmap']" in str(w.message) + and 'issue #1984' in str(w.message) + ] + assert matched, ( + "Expected a DeprecationWarning mentioning attrs['cmap'] and " + "issue #1984; got: " + f"{[str(w.message) for w in record]}" + ) + + +def test_deprecated_colormap_attrs_still_emitted(tmp_path): + """During the deprecation window the matplotlib variants still + land on ``DataArray.attrs``. + + Removing emission is a follow-up PR; this test pins the current + contract that callers can still read the attrs while migrating. + """ + path = _palette_fixture(tmp_path, name='palette_still_emitted.tif') + + with warnings.catch_warnings(): + warnings.simplefilter('ignore', DeprecationWarning) + da = open_geotiff(path) + + assert 'colormap_rgba' in da.attrs, ( + "attrs['colormap_rgba'] should still be emitted during the " + "deprecation window; got attrs: " + f"{sorted(da.attrs.keys())}" + ) + if _HAS_MATPLOTLIB: + assert 'cmap' in da.attrs, ( + "attrs['cmap'] should still be emitted during the deprecation " + "window when matplotlib is installed; got attrs: " + f"{sorted(da.attrs.keys())}" + ) + + +def test_plain_colormap_attr_not_deprecated(tmp_path): + """The canonical ``attrs['colormap']`` (raw uint16 triples from + tag 320) is unaffected: it still lands in attrs and does NOT carry + a DeprecationWarning mentioning its key. + + The matplotlib-derived variants share the same Photometric=3 gate, + so warnings for those two fire on the same read. The check below + targets ``"attrs['colormap']"`` as a whole-token substring (with + its closing bracket) so it does not match the deprecated variants. + """ + path = _palette_fixture(tmp_path, name='palette_plain_colormap.tif') + + with warnings.catch_warnings(record=True) as record: + warnings.simplefilter('always') + da = open_geotiff(path) + + # Plain ``colormap`` (canonical tier) is present. + assert 'colormap' in da.attrs, ( + "attrs['colormap'] must still be emitted; it round-trips via " + "_merge_friendly_extra_tags and is the canonical replacement " + "for the deprecated matplotlib variants. attrs: " + f"{sorted(da.attrs.keys())}" + ) + # Raw uint16 triples: 3 * 256 = 768 entries. + assert len(da.attrs['colormap']) == 768 + + # No DeprecationWarning targets the plain ``colormap`` key. The + # deprecated variant warnings reference ``attrs['colormap']`` in + # their migration hint, so we look for the "is deprecated" verb + # immediately after the key to find a warning whose *subject* is + # the plain key. + bad = [ + w for w in record + if issubclass(w.category, DeprecationWarning) + and "attrs['colormap'] is deprecated" in str(w.message) + ] + assert not bad, ( + "attrs['colormap'] must not trigger a DeprecationWarning; only " + "the matplotlib variants (cmap, colormap_rgba) are deprecated. " + f"Got: {[str(w.message) for w in bad]}" + ) diff --git a/xrspatial/geotiff/tests/test_features.py b/xrspatial/geotiff/tests/test_features.py index c36552afc..40c56d83c 100644 --- a/xrspatial/geotiff/tests/test_features.py +++ b/xrspatial/geotiff/tests/test_features.py @@ -2426,6 +2426,13 @@ def add_shorts(tag, vals): return bytes(out) +@pytest.mark.filterwarnings( + # PR 7 of issue #1984 deprecates attrs['cmap'] and + # attrs['colormap_rgba'] on palette-photometric reads. These tests + # exist specifically to exercise that path; the deprecation is + # locked in test_attrs_pr7_deprecate_colormap_variants_1984.py. + "ignore:.*attrs..(cmap|colormap_rgba)...is deprecated.*:DeprecationWarning" +) class TestPalette: def test_palette_8bit_read(self, tmp_path): diff --git a/xrspatial/geotiff/tests/test_metadata_round_trip_1484.py b/xrspatial/geotiff/tests/test_metadata_round_trip_1484.py index 0921e03fa..5248be4af 100644 --- a/xrspatial/geotiff/tests/test_metadata_round_trip_1484.py +++ b/xrspatial/geotiff/tests/test_metadata_round_trip_1484.py @@ -319,7 +319,16 @@ def test_colormap_round_trip(self, tmp_path): in_path = str(tmp_path / 'colormap_in_1484.tif') _make_palette_uint8_tiff(in_path, pixels, palette) - da = open_geotiff(in_path) + # Reading a Photometric=3 fixture trips the deprecation warnings + # for ``attrs['cmap']`` and ``attrs['colormap_rgba']`` (issue + # #1984 PR 7). Those are pinned in + # ``test_attrs_pr7_deprecate_colormap_variants_1984.py``; here + # we just want to verify ``attrs['colormap']`` still round-trips + # and the noise would otherwise leak into this file's report. + import warnings as _w + with _w.catch_warnings(): + _w.simplefilter('ignore', DeprecationWarning) + da = open_geotiff(in_path) assert da.dtype == np.uint8 assert 'colormap' in da.attrs # Raw uint16 ColorMap: 3 * 256 = 768 entries From 0b5797a017b80d534d515dffbe064f84a62545b6 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 20:57:24 -0700 Subject: [PATCH 2/3] geotiff: factor shared _emit_deprecated_geokey_attr helper PR review on #2014 flagged that the three colormap deprecation sites copy-paste the same nine-line ``warnings.warn(...)`` block with only the attr name and migration hint varying. Sibling PRs #2010 / #2013 have the same shape; PR #2011 already factors a slice-specific helper. Introduce a generic helper in ``_attrs.py`` so all four PR 7 slices can share it. ``_emit_deprecated_geokey_attr(attrs, name, value, *, reason, migration=None)`` builds the canonical warning text shape: xrspatial.geotiff: attrs[''] is deprecated; . [.] It will be removed in a future release. See issue #1984. then sets ``attrs[name] = value`` (read-side emission still alive for the deprecation window). ``stacklevel=2`` lands the warning at the caller of ``_populate_attrs_from_geo_info``; ``DeprecationWarning`` is silenced for library code by default in Python's filters, so this is mainly visible to test runners and developers who opt in. The colormap-variants deprecation now routes ``cmap`` and the two ``colormap_rgba`` branches (matplotlib present + ImportError fallback) through the helper, replacing 27 lines of copy-paste with three calls. The emitted warning text is byte-identical to the previous inline strings, so the existing tests (``pytest.warns`` plus the TestPalette / test_metadata_round_trip filter regexes) keep passing unchanged. Sibling PRs #2010 and #2013 can adopt the helper on rebase; #2011's existing geographic-specific helper can be inlined onto this one in a follow-up. Test plan: - pytest xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py -- 4 passed. - pytest xrspatial/geotiff/tests/test_attrs_contract_*_1984.py xrspatial/geotiff/tests/test_metadata_round_trip_1484.py -- 57 passed. - pytest xrspatial/geotiff/tests/test_features.py::TestPalette -- 7 passed. --- xrspatial/geotiff/_attrs.py | 91 +++++++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 28 deletions(-) diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index 597e66be9..2978430c0 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -136,6 +136,49 @@ _RESOLUTION_UNIT_IDS = {'none': 1, 'inch': 2, 'centimeter': 3} +def _emit_deprecated_geokey_attr( + attrs: dict, + name: str, + value, + *, + reason: str, + migration: str | None = None, +) -> None: + """Set ``attrs[name] = value`` and fire a ``DeprecationWarning``. + + Shared shape for the issue #1984 PR 7 deprecation slices (vertical, + geographic, projected, colormap variants). The attr is still emitted + during the warning-only phase so existing consumers keep working; + removal lands in a follow-up release. + + ``reason`` is the one-liner explaining why the attr cannot + round-trip (e.g. "the writer cannot reconstruct it from the + canonical CRS"). ``migration`` is an optional one-liner pointing + at the canonical replacement. + + Warning text shape:: + + xrspatial.geotiff: attrs[''] is deprecated; + It will be removed in a future release. See + issue #1984. + + ``stacklevel=2`` aims the warning at the caller of + ``_populate_attrs_from_geo_info``, which is the closest frame the + user controls today. Python silences ``DeprecationWarning`` for + library code by default, so the warning is visible mainly to test + runners and developers who opt in with ``-W default::DeprecationWarning``. + """ + parts = [ + f"xrspatial.geotiff: attrs[{name!r}] is deprecated;", + reason.rstrip('.') + '.', + ] + if migration: + parts.append(migration.rstrip('.') + '.') + parts.append("It will be removed in a future release. See issue #1984.") + warnings.warn(' '.join(parts), 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). @@ -295,40 +338,32 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None geo_info.resolution_unit, str(geo_info.resolution_unit)) if geo_info.colormap is not None: + _colormap_reason = ( + "the writer cannot set Photometric=3 so it does not round-trip" + ) + _colormap_migration = ( + "Construct a ListedColormap from attrs['colormap'] in caller " + "code if needed" + ) try: from matplotlib.colors import ListedColormap - warnings.warn( - "xrspatial.geotiff: attrs['cmap'] is deprecated; the writer " - "cannot set Photometric=3 so it does not round-trip. " - "Construct a ListedColormap from attrs['colormap'] in caller " - "code if needed. It will be removed in a future release. " - "See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'cmap', + ListedColormap(geo_info.colormap, name='tiff_palette'), + reason=_colormap_reason, + migration=_colormap_migration, ) - attrs['cmap'] = ListedColormap( - geo_info.colormap, name='tiff_palette') - warnings.warn( - "xrspatial.geotiff: attrs['colormap_rgba'] is deprecated; " - "the writer cannot set Photometric=3 so it does not " - "round-trip. Construct a ListedColormap from " - "attrs['colormap'] in caller code if needed. It will be " - "removed in a future release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'colormap_rgba', geo_info.colormap, + reason=_colormap_reason, + migration=_colormap_migration, ) - attrs['colormap_rgba'] = geo_info.colormap except ImportError: - warnings.warn( - "xrspatial.geotiff: attrs['colormap_rgba'] is deprecated; " - "the writer cannot set Photometric=3 so it does not " - "round-trip. Construct a ListedColormap from " - "attrs['colormap'] in caller code if needed. It will be " - "removed in a future release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'colormap_rgba', geo_info.colormap, + reason=_colormap_reason, + migration=_colormap_migration, ) - attrs['colormap_rgba'] = geo_info.colormap if geo_info.extra_tags is not None: for _tag_id, _tt, _tc, _tv in geo_info.extra_tags: From a43e05685d582964d00af0e41834c716fc6e4d67 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 21:02:09 -0700 Subject: [PATCH 3/3] geotiff: address self-review on shared deprecation helper Self-review on PR #2014 flagged one blocker and two suggestions on the shared-helper refactor. This commit addresses them. Blocker -- ``stacklevel`` regression (``_attrs.py:179``): Wrapping the inline ``warnings.warn(..., stacklevel=2)`` call in a helper shifted the warning location by one frame; the warning was surfacing at ``_attrs.py`` (the helper call site) instead of at the backend that called ``_populate_attrs_from_geo_info``. Bumped ``stacklevel=2`` to ``stacklevel=3`` so the warning is attributed to the backend frame, matching the pre-refactor behaviour (verified empirically: warning now surfaces at ``xrspatial/geotiff/__init__.py:518`` instead of ``_attrs.py:350``). Suggestion -- helper name (``_attrs.py:152``): Renamed ``_emit_deprecated_geokey_attr`` to ``_emit_deprecated_attr`` because the colormap variants (cmap, colormap_rgba) come from TIFF tag 320 (ColorMap), not from a GeoKey. The new name covers every PR 7 slice; docstring notes why the ``_geokey_`` qualifier was dropped. Suggestion -- user-guide doc: Added a new "Deprecated keys" section to ``docs/source/user_guide/attrs_contract.rst`` so users reading the contract page see the new tier alongside the existing canonical / compatibility-alias / pass-through tiers. Mirrors the module docstring. Suggestion -- direct unit test for the helper: Added ``test_emit_deprecated_attr_with_migration_text`` and ``test_emit_deprecated_attr_without_migration_text``. The first pins the exact warning text shape; the second pins the ``migration=None`` branch which the existing colormap tests do not exercise. Nit -- migration hints: Lifted ``_colormap_reason`` / ``_colormap_migration`` to module- level constants and split them into ``_DEPRECATED_COLORMAP_REASON``, ``_DEPRECATED_CMAP_MIGRATION``, and ``_DEPRECATED_COLORMAP_RGBA_MIGRATION``. The new ``colormap_rgba``-specific migration ("Reshape attrs['colormap'] to (n_colors, 3) and append an alpha channel") is more direct than the previous shared ``ListedColormap`` hint, which only matched ``cmap`` cleanly. Test plan: - pytest test_attrs_pr7_deprecate_colormap_variants_1984.py (6 passed) - pytest test_attrs_contract_*_1984.py test_metadata_round_trip_1484.py test_features.py::TestPalette -- 70 passed total --- docs/source/user_guide/attrs_contract.rst | 31 ++++++++- xrspatial/geotiff/_attrs.py | 69 +++++++++++-------- ...rs_pr7_deprecate_colormap_variants_1984.py | 69 +++++++++++++++++++ 3 files changed, 138 insertions(+), 31 deletions(-) diff --git a/docs/source/user_guide/attrs_contract.rst b/docs/source/user_guide/attrs_contract.rst index ba79f4243..f9f193137 100644 --- a/docs/source/user_guide/attrs_contract.rst +++ b/docs/source/user_guide/attrs_contract.rst @@ -174,13 +174,38 @@ must not assume a specific pass-through key survives a round-trip. * - ``colormap`` - tuple - Raw ``ColorMap`` TIFF tag (tag id 320) values. + + +Deprecated keys +=============== + +These keys are still emitted on read for one release cycle, but each +emission triggers a ``DeprecationWarning``. The writer cannot +reconstruct them from canonical attrs, so they do not round-trip. +Callers should migrate to the canonical alternative listed below +before the warning-only window closes. See issue #1984. + +.. list-table:: + :header-rows: 1 + :widths: 22 18 60 + + * - Key + - Type + - Definition and migration * - ``colormap_rgba`` - array - - Decoded RGBA colormap, when one is present. + - Decoded RGBA colormap. Emitted on read when the file's + photometric interpretation is ``Photometric == 3`` (palette). + The writer cannot set ``Photometric == 3`` so the attr does + not round-trip. Reshape ``attrs['colormap']`` to + ``(n_colors, 3)`` and append an alpha channel in caller code + if needed. * - ``cmap`` - ``matplotlib.colors.ListedColormap`` - - Matplotlib colormap built from ``colormap_rgba``. Present only - when matplotlib is importable. + - Matplotlib colormap built from the palette. Same + ``Photometric == 3`` gate, same round-trip gap. Construct a + ``ListedColormap`` from ``attrs['colormap']`` in caller code + if needed. Round-trip invariants diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index 2978430c0..d5ae7a11d 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -81,8 +81,9 @@ - ``colormap_rgba``: RGBA palette array, only emitted on read when the source file is Photometric==3 (palette). The writer never selects - Photometric=3, so this attr does not round-trip. Construct an RGBA - palette from ``attrs['colormap']`` in caller code if needed. + Photometric=3, so this attr does not round-trip. Reshape + ``attrs['colormap']`` to ``(n_colors, 3)`` and append an alpha + channel in caller code if needed. - ``cmap``: matplotlib ``ListedColormap`` built from the palette. Same Photometric==3 gate, same round-trip gap. Construct a ``ListedColormap`` from ``attrs['colormap']`` in caller code if @@ -136,7 +137,23 @@ _RESOLUTION_UNIT_IDS = {'none': 1, 'inch': 2, 'centimeter': 3} -def _emit_deprecated_geokey_attr( +# Shared wording for the colormap-variants slice of the PR 7 deprecation +# series. Kept at module scope so the helper call sites stay one-liners +# and so sibling slices can mirror the same constant-style. +_DEPRECATED_COLORMAP_REASON = ( + "the writer cannot set Photometric=3 so it does not round-trip" +) +_DEPRECATED_CMAP_MIGRATION = ( + "Construct a ListedColormap from attrs['colormap'] in caller code " + "if needed" +) +_DEPRECATED_COLORMAP_RGBA_MIGRATION = ( + "Reshape attrs['colormap'] to (n_colors, 3) and append an alpha " + "channel in caller code if needed" +) + + +def _emit_deprecated_attr( attrs: dict, name: str, value, @@ -147,9 +164,11 @@ def _emit_deprecated_geokey_attr( """Set ``attrs[name] = value`` and fire a ``DeprecationWarning``. Shared shape for the issue #1984 PR 7 deprecation slices (vertical, - geographic, projected, colormap variants). The attr is still emitted - during the warning-only phase so existing consumers keep working; - removal lands in a follow-up release. + geographic, projected, and colormap variants). The attr is still + emitted during the warning-only phase so existing consumers keep + working; removal lands in a follow-up release. Naming: kept generic + (not ``_emit_deprecated_geokey_attr``) because the colormap variants + come from the TIFF ColorMap tag (320) rather than a GeoKey. ``reason`` is the one-liner explaining why the attr cannot round-trip (e.g. "the writer cannot reconstruct it from the @@ -162,11 +181,12 @@ def _emit_deprecated_geokey_attr( It will be removed in a future release. See issue #1984. - ``stacklevel=2`` aims the warning at the caller of - ``_populate_attrs_from_geo_info``, which is the closest frame the - user controls today. Python silences ``DeprecationWarning`` for - library code by default, so the warning is visible mainly to test - runners and developers who opt in with ``-W default::DeprecationWarning``. + ``stacklevel=3`` is set so the emitted warning is attributed to + the caller of ``_populate_attrs_from_geo_info`` (the backend that + is reading the file), not to the helper or to the helper's caller. + Python silences ``DeprecationWarning`` for library code by default, + so the warning is visible mainly to test runners and to developers + who opt in with ``-W default::DeprecationWarning``. """ parts = [ f"xrspatial.geotiff: attrs[{name!r}] is deprecated;", @@ -175,7 +195,7 @@ def _emit_deprecated_geokey_attr( if migration: parts.append(migration.rstrip('.') + '.') parts.append("It will be removed in a future release. See issue #1984.") - warnings.warn(' '.join(parts), DeprecationWarning, stacklevel=2) + warnings.warn(' '.join(parts), DeprecationWarning, stacklevel=3) attrs[name] = value @@ -338,31 +358,24 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None geo_info.resolution_unit, str(geo_info.resolution_unit)) if geo_info.colormap is not None: - _colormap_reason = ( - "the writer cannot set Photometric=3 so it does not round-trip" - ) - _colormap_migration = ( - "Construct a ListedColormap from attrs['colormap'] in caller " - "code if needed" - ) try: from matplotlib.colors import ListedColormap - _emit_deprecated_geokey_attr( + _emit_deprecated_attr( attrs, 'cmap', ListedColormap(geo_info.colormap, name='tiff_palette'), - reason=_colormap_reason, - migration=_colormap_migration, + reason=_DEPRECATED_COLORMAP_REASON, + migration=_DEPRECATED_CMAP_MIGRATION, ) - _emit_deprecated_geokey_attr( + _emit_deprecated_attr( attrs, 'colormap_rgba', geo_info.colormap, - reason=_colormap_reason, - migration=_colormap_migration, + reason=_DEPRECATED_COLORMAP_REASON, + migration=_DEPRECATED_COLORMAP_RGBA_MIGRATION, ) except ImportError: - _emit_deprecated_geokey_attr( + _emit_deprecated_attr( attrs, 'colormap_rgba', geo_info.colormap, - reason=_colormap_reason, - migration=_colormap_migration, + reason=_DEPRECATED_COLORMAP_REASON, + migration=_DEPRECATED_COLORMAP_RGBA_MIGRATION, ) if geo_info.extra_tags is not None: diff --git a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py index e8502c2f8..49d382e57 100644 --- a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py +++ b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py @@ -270,3 +270,72 @@ def test_plain_colormap_attr_not_deprecated(tmp_path): "the matplotlib variants (cmap, colormap_rgba) are deprecated. " f"Got: {[str(w.message) for w in bad]}" ) + + +# --------------------------------------------------------------------------- +# Direct unit tests for the shared helper ``_emit_deprecated_attr``. +# +# The colormap-variants tests above exercise the helper indirectly through +# the full ``open_geotiff`` path. The two cases below cover the message +# shape and the optional-``migration`` branch directly so the contract +# stays pinned even if every consumer of the helper changes. +# --------------------------------------------------------------------------- + + +def test_emit_deprecated_attr_with_migration_text(): + """``_emit_deprecated_attr`` builds the documented warning shape and + sets the attr value.""" + from xrspatial.geotiff._attrs import _emit_deprecated_attr + + attrs: dict = {} + with warnings.catch_warnings(record=True) as record: + warnings.simplefilter('always') + _emit_deprecated_attr( + attrs, + 'sample_attr', + 42, + reason="the writer cannot reconstruct it from canonical state", + migration="Use attrs['other_attr'] instead", + ) + + dep = [ + w for w in record + if issubclass(w.category, DeprecationWarning) + ] + assert len(dep) == 1 + msg = str(dep[0].message) + assert msg == ( + "xrspatial.geotiff: attrs['sample_attr'] is deprecated; " + "the writer cannot reconstruct it from canonical state. " + "Use attrs['other_attr'] instead. " + "It will be removed in a future release. See issue #1984." + ) + assert attrs == {'sample_attr': 42} + + +def test_emit_deprecated_attr_without_migration_text(): + """``migration=None`` omits the migration sentence cleanly (no dangling + space or empty clause).""" + from xrspatial.geotiff._attrs import _emit_deprecated_attr + + attrs: dict = {} + with warnings.catch_warnings(record=True) as record: + warnings.simplefilter('always') + _emit_deprecated_attr( + attrs, + 'foo', + 'bar', + reason="some reason", + ) + + dep = [ + w for w in record + if issubclass(w.category, DeprecationWarning) + ] + assert len(dep) == 1 + msg = str(dep[0].message) + assert msg == ( + "xrspatial.geotiff: attrs['foo'] is deprecated; some reason. " + "It will be removed in a future release. See issue #1984." + ) + assert attrs == {'foo': 'bar'}