diff --git a/CHANGELOG.md b/CHANGELOG.md index b2c6989b8..095d4de52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,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/docs/source/user_guide/attrs_contract.rst b/docs/source/user_guide/attrs_contract.rst index 21a2a2b13..ac69e8f36 100644 --- a/docs/source/user_guide/attrs_contract.rst +++ b/docs/source/user_guide/attrs_contract.rst @@ -180,15 +180,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. Emitted only when the file's photometric - interpretation is ``Photometric == 3`` (palette). + - 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 and the same ``Photometric == 3`` - gate is satisfied. + - 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 908863163..47d5a6b9c 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -63,8 +63,8 @@ - ``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): @@ -103,6 +103,17 @@ reason as ``vertical_crs``. - ``vertical_units``: VerticalUnitsGeoKey value. Same deprecation reason as ``vertical_crs``. +Colormap variants (different root cause: photometric gate, not GeoKey): + +- ``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. 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 + needed. Migration recipe (the canonical replacement is ``crs`` / ``crs_wkt`` plus a one-liner with :mod:`pyproj` when a derived value is needed):: @@ -208,6 +219,25 @@ ) +# Shared wording for the colormap-variants slice of the PR 7 deprecation +# series. The root cause is a different one (the writer cannot set +# ``Photometric=3``) so the GeoKey-tier reason templates above don't +# fit; these constants are spliced into ``_emit_deprecated_attr`` with +# a per-attr migration recipe so users see how to derive an RGBA palette +# or matplotlib ``ListedColormap`` from canonical ``attrs['colormap']``. +_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 _deprecated_geokey_warning(name: str, *, reason: str) -> str: """Warning text for a deprecated GeoKey-derived attr. @@ -332,6 +362,54 @@ def _emit_deprecated_geographic_geokey(attrs: dict, name: str, value) -> None: ) +def _emit_deprecated_attr( + attrs: dict, + name: str, + value, + *, + reason: str, + migration: str | None = None, +) -> None: + """Emit a deprecated attr with a ``DeprecationWarning`` and a + per-attr migration recipe. + + Sibling of :func:`_emit_deprecated_geokey_attr` that adds support + for an optional ``migration`` clause spliced into the warning. The + GeoKey-tier helper uses a fixed sentence ("... so it will not + round-trip ..."); the colormap-variants tier needs to point users + at how to derive an RGBA palette or matplotlib ``ListedColormap`` + from canonical ``attrs['colormap']``, which doesn't fit that + template. A follow-up may unify the two helpers; for now they live + side-by-side because the warning-text contracts are pinned by + separate test suites and converging them is out of scope for the + colormap slice. + + Warning text shape:: + + xrspatial.geotiff: attrs[''] is deprecated; + It will be removed in a future release. See + issue #1984. + + The ``stacklevel`` is taken from + :func:`_stacklevel_to_external_caller` so the warning is attributed + to the user's call site, matching the GeoKey-tier slices. + """ + 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=_stacklevel_to_external_caller(), + ) + 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). @@ -513,11 +591,23 @@ 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 - attrs['cmap'] = ListedColormap( - geo_info.colormap, name='tiff_palette') - attrs['colormap_rgba'] = geo_info.colormap + _emit_deprecated_attr( + attrs, 'cmap', + ListedColormap(geo_info.colormap, name='tiff_palette'), + reason=_DEPRECATED_COLORMAP_REASON, + migration=_DEPRECATED_CMAP_MIGRATION, + ) + _emit_deprecated_attr( + attrs, 'colormap_rgba', geo_info.colormap, + reason=_DEPRECATED_COLORMAP_REASON, + migration=_DEPRECATED_COLORMAP_RGBA_MIGRATION, + ) except ImportError: - attrs['colormap_rgba'] = geo_info.colormap + _emit_deprecated_attr( + attrs, 'colormap_rgba', geo_info.colormap, + reason=_DEPRECATED_COLORMAP_REASON, + migration=_DEPRECATED_COLORMAP_RGBA_MIGRATION, + ) if geo_info.extra_tags is not None: for _tag_id, _tt, _tc, _tv in geo_info.extra_tags: 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..49d382e57 --- /dev/null +++ b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_colormap_variants_1984.py @@ -0,0 +1,341 @@ +"""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]}" + ) + + +# --------------------------------------------------------------------------- +# 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'} 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