geotiff: deprecate projected-CRS GeoKey attrs in DataArray.attrs (#1984)#2013
Merged
Conversation
The pass-through tier of the attrs contract (#1984, locked in #2004) flagged ``linear_units`` and ``projection_code`` as dropped on a write + read round-trip: ``build_geo_tags`` only emits the primary ``GEOKEY_PROJECTED_CS_TYPE`` and never the secondary projected GeoKeys (``GEOKEY_PROJECTION``, ``GEOKEY_PROJ_LINEAR_UNITS``), so the reader has nothing to rebuild the attrs from. Wrap the two emission sites in ``_populate_attrs_from_geo_info`` so each one fires a ``DeprecationWarning`` referencing #1984. The attrs are still emitted during the deprecation cycle; removal is a follow-up. Docstring: move both keys from the pass-through tier to a new "Deprecated" section. A sibling PR is adding the same section for geographic GeoKey attrs and may collide on this docstring block. Tests: ``test_attrs_pr7_deprecate_projected_1984.py`` covers the two warnings plus a guard that the attrs still populate during the deprecation cycle.
3 tasks
Contributor
Author
PR Review: geotiff: deprecate projected-CRS GeoKey attrsBlockers (must fix before merge)None. Suggestions (should fix, not blocking)
Nits (optional improvements)
What looks good
Checklist
|
3 tasks
# Conflicts: # CHANGELOG.md # xrspatial/geotiff/_attrs.py
brendancol
added a commit
that referenced
this pull request
May 18, 2026
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['<name>'] is deprecated; <reason>. [<migration>.] 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.
4 tasks
brendancol
added a commit
that referenced
this pull request
May 18, 2026
Address review S1 + N2 on PR #2011. N2: promote ``_emit_deprecated_geographic_geokey`` to a generic ``_emit_deprecated_geokey_attr(attrs, name, value, *, reason)`` shared by the geographic, projected, and vertical deprecation tiers (introduced by sibling PRs #2010 and #2013 as inline ``warnings.warn`` calls during the rebase onto current main). The per-category reason clauses live in two module-level constants (``_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS`` for geographic / projected; ``_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS`` for vertical), which keeps the wording in lockstep and lets the existing ``test_warning_message_format`` assertion continue to pin the canonical text verbatim. ``_emit_deprecated_geographic_geokey`` is kept as a thin shim so the geographic call sites stay readable; the projected and vertical sites in ``_populate_attrs_from_geo_info`` now call the generic helper directly and inherit the external-caller stacklevel walk from commit B for free. S1: the rebased docstring "Deprecated" section already grew a migration recipe in the conflict resolution; this commit additionally mirrors the recipe into ``docs/source/user_guide/attrs_contract.rst`` so the user-facing docs carry the same pyproj one-liners (``crs.ellipsoid.inverse_flattening``, ``crs.datum.to_epsg()`` etc.) that the docstring lists. No new tests. The existing 17 deprecation tests plus the 42 contract tests all still pass; the warning text and category are unchanged. Sibling PRs #2010 and #2013 are already merged on main, so no sibling rebase is needed -- this PR is the consolidation point.
brendancol
added a commit
that referenced
this pull request
May 18, 2026
Two textual conflicts in files where the colormap-variants slice and the sibling PR 7 slices (#2010 vertical, #2013 projected, #2012 doc nits) both edited the same regions: xrspatial/geotiff/_attrs.py (module docstring) Origin/main moved linear_units / projection_code / vertical_crs / vertical_citation / vertical_units from the pass-through section into a new "Deprecated" section, and kept the historical combined ``colormap, colormap_rgba, cmap`` line in pass-through. HEAD split that line: ``colormap`` stays in pass-through (with detail about the uint16 RGB triples it carries), ``colormap_rgba`` and ``cmap`` move to the "Deprecated" section. Resolution: keep main's five deprecated entries and add HEAD's two below them, so all seven PR 7 deprecations appear in one section in the order the slices were authored. docs/source/user_guide/attrs_contract.rst Origin/main (via #2012) tightened the colormap_rgba and cmap row text in the pass-through table but left both keys in the pass- through tier. HEAD moved both rows into a new "Deprecated keys" section with the deprecation rationale and a per-row migration recipe. Resolution: keep HEAD's "Deprecated keys" section as the new home for both keys; the pass-through table no longer lists them (already resolved by the auto-merge on adjacent lines). Pre-existing inconsistency not addressed here: origin/main now marks ``linear_units``, ``projection_code``, and the vertical-CRS attrs as deprecated in the Python module docstring but still lists them under "Pass-through keys" in the RST user-guide page. That gap was introduced by #2010 / #2013, which did not update the RST. Fixing it is scope creep for this PR; flag for the next #1984 follow-up. Test plan: - pytest test_attrs_pr7_deprecate_colormap_variants_1984.py (6 passed) - pytest test_attrs_pr7_deprecate_vertical_1984.py (4 passed) - pytest test_attrs_pr7_deprecate_projected_1984.py (3 passed) - pytest test_attrs_contract_*_1984.py test_metadata_round_trip_1484.py test_features.py::TestPalette -- 77 passed total, no regressions.
brendancol
added a commit
that referenced
this pull request
May 18, 2026
) (#2011) * 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. * geotiff: tighten PR7 geographic deprecation test parametrisation (#1984) Address review S2 + N1 on PR #2011. S2: drop the redundant ``field`` column from ``_DEPRECATED_CASES``. Every entry had ``attr == field`` because every deprecated attr is stored on ``GeoInfo`` under the same name it lands in ``attrs`` under. Collapse to ``(attr, value)`` and update the two parametrised tests to match. N1: add ``test_deprecated_cases_has_no_duplicates`` so a duplicate row in ``_DEPRECATED_CASES`` can no longer be silently absorbed by the set-based coverage check. Test count goes 15 -> 16. * geotiff: walk past internal frames in deprecation warning stacklevel (#1984) Address review S3 on PR #2011. The geographic-GeoKey deprecation helper passed ``stacklevel=2`` to ``warnings.warn``, which pointed at ``_populate_attrs_from_geo_info`` rather than the user's ``open_geotiff(...)`` call. The exact distance to the user frame also varies by backend: the numpy path adds three internal frames, the dask path adds four. A fixed integer can only be right for one of those paths. Replace the fixed level with a frame walk (``_stacklevel_to_external_caller``) that skips every ``xrspatial.geotiff*`` frame -- minus ``xrspatial.geotiff.tests`` so the unit tests can still pose as external callers -- and returns the matching ``stacklevel``. The new ``test_warning_stacklevel_points_at_caller_file`` pins the attribution. Today the warning category is ``DeprecationWarning``, which Python silences by default for library code, so the stacklevel mostly affects test output. Get it right now so a later switch to ``FutureWarning`` does not surface the warning as if it came from ``_attrs.py``. Scope note: the inline ``warnings.warn`` calls for the projected and vertical tiers (still pass ``stacklevel=2``) are left alone in this commit; the next commit folds them into the generic helper and picks up the new walk transparently. Test count goes 16 -> 17. * geotiff: fold deprecation emission into a generic GeoKey helper (#1984) Address review S1 + N2 on PR #2011. N2: promote ``_emit_deprecated_geographic_geokey`` to a generic ``_emit_deprecated_geokey_attr(attrs, name, value, *, reason)`` shared by the geographic, projected, and vertical deprecation tiers (introduced by sibling PRs #2010 and #2013 as inline ``warnings.warn`` calls during the rebase onto current main). The per-category reason clauses live in two module-level constants (``_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS`` for geographic / projected; ``_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS`` for vertical), which keeps the wording in lockstep and lets the existing ``test_warning_message_format`` assertion continue to pin the canonical text verbatim. ``_emit_deprecated_geographic_geokey`` is kept as a thin shim so the geographic call sites stay readable; the projected and vertical sites in ``_populate_attrs_from_geo_info`` now call the generic helper directly and inherit the external-caller stacklevel walk from commit B for free. S1: the rebased docstring "Deprecated" section already grew a migration recipe in the conflict resolution; this commit additionally mirrors the recipe into ``docs/source/user_guide/attrs_contract.rst`` so the user-facing docs carry the same pyproj one-liners (``crs.ellipsoid.inverse_flattening``, ``crs.datum.to_epsg()`` etc.) that the docstring lists. No new tests. The existing 17 deprecation tests plus the 42 contract tests all still pass; the warning text and category are unchanged. Sibling PRs #2010 and #2013 are already merged on main, so no sibling rebase is needed -- this PR is the consolidation point.
brendancol
added a commit
that referenced
this pull request
May 18, 2026
…1984) (#2014) * 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. * 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['<name>'] is deprecated; <reason>. [<migration>.] 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. * 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
This was referenced May 18, 2026
brendancol
added a commit
that referenced
this pull request
May 18, 2026
…2015) The four PR 7 deprecation slices (#2010 vertical, #2011 geographic, #2013 projected, #2014 colormap variants) moved 13 attrs from the pass-through tier to a new deprecated tier in ``xrspatial/geotiff/_attrs.py`` but did not update the user-guide page, which left ``docs/source/user_guide/attrs_contract.rst`` listing those attrs as round-trip-best-effort while the Python code fires a DeprecationWarning every time one is emitted. This is docs-only: * Trim ``Pass-through keys`` table to ``image_description``, ``extra_samples``, ``colormap`` -- the three attrs that genuinely round-trip via ``_merge_friendly_extra_tags``. The 11 GeoKey- derived attrs plus the two colormap variants moved out. * Merge the two separate deprecated sections that existed pre-PR (``Deprecated keys`` for colormap variants, ``Deprecated GeoKey attrs`` for the GeoKey slices) into one ``Deprecated keys`` section with two subsections: GeoKey-derived attrs (with the shared pyproj migration recipe) and matplotlib colormap variants (with per-row migration in a list-table). * Update the page's opening summary from "three tiers" to "four tiers" so the first paragraph matches the table-of-contents. * Update the ``Round-trip invariants`` section: the pass-through subsection now reflects that only three attrs live there and all three round-trip; add a deprecated subsection that explains the warning-only phase and points at the future contract-version bump to 2 when removal lands. No production-code change. Verified the file still parses with docutils (Sphinx-specific ``:func:`` / ``:mod:`` role warnings are the same pattern used across the rest of the docs). Refs #1984.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of the issue #1984 attrs contract rollout. PR 6 (#2004) locked which pass-through attrs survive a write + read round-trip. Two projected-CRS GeoKey attrs are dropped on round-trip because
xrspatial.geotiff._geotags.build_geo_tagsonly emits the primaryGEOKEY_PROJECTED_CS_TYPE(3072) and never the secondary projected GeoKeys:linear_units(ProjLinearUnitsGeoKey, 3076)projection_code(ProjectionGeoKey, 3074)The reader rebuilds the attrs from those secondary GeoKeys on read, but the writer never emits them, so a write + read cycle silently drops both values.
This PR starts a one-release-cycle deprecation:
_populate_attrs_from_geo_infostill sets both attrs when the source file carries the underlying GeoKey, but each emission now fires aDeprecationWarningreferencing issue geotiff: define a public contract for DataArray attrs (canonical / alias / pass-through) #1984._attrs.pymodule docstring moves the two keys from the "Best-effort pass-through" list to a new "Deprecated" section.CHANGELOG.mdhas an Unreleased entry.Removal of the emission is a separate follow-up after the deprecation cycle.
Coordination / rebase risk
A sibling PR (
1984-pr7-deprecate-geographic-geokeys) is adding the same "Deprecated" section to the same docstring block for the geographic-CRS attrs. Whichever branch lands second will hit a docstring conflict and need to merge its bullet list into the existing "Deprecated" section. The conflict is mechanical; behaviour on the two PRs is independent.Test plan
pytest xrspatial/geotiff/tests/test_attrs_pr7_deprecate_projected_1984.py-- 3 passedpytest xrspatial/geotiff/tests/test_attrs_contract_*1984.py-- 47 passed (canonical / aliases / version / pass-through tiers still hold)pytest xrspatial/geotiff/tests/-- pre-existing GPU failures only; the 3 new tests pass and no new failures appear on CPU paths