From 0d79dbf2fb8a3d9e977ef145437cf9a7607487a0 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 20:37:40 -0700 Subject: [PATCH 1/4] 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. --- CHANGELOG.md | 1 + xrspatial/geotiff/_attrs.py | 125 +++++++++++-- ...est_attrs_pr7_deprecate_geographic_1984.py | 164 ++++++++++++++++++ 3 files changed, 273 insertions(+), 17 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cc1b7eaa5..b2c6989b8 100644 --- a/CHANGELOG.md +++ b/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) - Deprecate read-side emission of projected-CRS GeoKey attrs (linear_units, projection_code); 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) - Deprecate read-side emission of vertical-CRS GeoKey attrs (vertical_crs, vertical_citation, vertical_units); the writer does not emit vertical GeoKeys 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 diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index f4324d5da..38800ed32 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -61,35 +61,75 @@ Best-effort pass-through (preserved when the writer can reconstruct from canonical state, otherwise dropped on round-trip): +- ``image_description``: TIFF ImageDescription tag. +- ``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 CRS GeoKey and citation for +each CRS axis (horizontal / projected / vertical), so the secondary +GeoKeys these attrs derive 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. + +Geographic-CRS GeoKey attrs: + - ``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. -- ``image_description``: TIFF ImageDescription tag. -- ``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): +Projected-CRS GeoKey attrs: - ``linear_units``: ProjLinearUnitsGeoKey value. The writer's ``build_geo_tags`` only emits the primary ``GEOKEY_PROJECTED_CS_TYPE`` and never the secondary projected GeoKeys, so this attr cannot be - reconstructed on round-trip. Read-side emission triggers a - ``DeprecationWarning`` for one release cycle before removal. + reconstructed on round-trip. - ``projection_code``: ProjectionGeoKey value. Same root cause as ``linear_units``: the writer never emits the underlying GeoKey, so - the value cannot survive a round-trip. Read-side emission triggers a - ``DeprecationWarning`` for one release cycle before removal. + the value cannot survive a round-trip. + +Vertical-CRS GeoKey attrs: + - ``vertical_crs``: VerticalCSTypeGeoKey value. The writer never emits - the vertical GeoKey block, so this attr cannot round-trip. It still - appears on read but triggers a ``DeprecationWarning``. + the vertical GeoKey block, so this attr cannot round-trip. - ``vertical_citation``: VerticalCitationGeoKey value. Same deprecation reason as ``vertical_crs``. - ``vertical_units``: VerticalUnitsGeoKey value. Same deprecation reason as ``vertical_crs``. + +Migration recipe (the canonical replacement is ``crs`` / ``crs_wkt`` +plus a one-liner with :mod:`pyproj` when a derived value is needed):: + + from pyproj import CRS + crs = CRS.from_wkt(attrs['crs_wkt']) # or CRS.from_epsg(attrs['crs']) + + # Geographic + crs.name # crs_name + crs.datum.to_epsg() # datum_code + crs.ellipsoid.semi_major_metre # semi_major_axis + crs.ellipsoid.inverse_flattening # inv_flattening + # geog_citation / angular_units: best-effort derive from + # ``crs`` / ``crs.axis_info``; the original GeoKey citation text + # is not generally recoverable. + + # Projected + crs.coordinate_system.axis_list[0].unit_name # linear_units + crs.to_epsg() # projection_code + + # Vertical + crs.sub_crs_list[-1].to_epsg() # vertical_crs + crs.sub_crs_list[-1].name # vertical_citation + crs.sub_crs_list[-1].axis_info[0].unit_name # vertical_units + +See ``docs/source/user_guide/attrs_contract.rst`` for the full +migration notes. """ from __future__ import annotations @@ -139,6 +179,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). @@ -254,13 +340,16 @@ 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: warnings.warn( "xrspatial.geotiff: attrs['linear_units'] is deprecated; " @@ -272,9 +361,11 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> 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: warnings.warn( "xrspatial.geotiff: attrs['projection_code'] is deprecated; " diff --git a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py new file mode 100644 index 000000000..89d1b5ea7 --- /dev/null +++ b/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_`` 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 From 05b0cff317b70cec67f41bdbf357b0397ced0cc7 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 21:01:19 -0700 Subject: [PATCH 2/4] 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. --- ...est_attrs_pr7_deprecate_geographic_1984.py | 41 +++++++++++++------ 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py index 89d1b5ea7..d792faded 100644 --- a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py +++ b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py @@ -39,19 +39,22 @@ from xrspatial.geotiff._geotags import GeoInfo -# (attr_name, GeoInfo field name, sample value). +# (attr_name, sample value). The attr name doubles as the ``GeoInfo`` +# field name: every deprecated attr in this PR is stored on +# :class:`GeoInfo` under the same identifier it lands in ``attrs`` +# under, so a single column is enough. # # 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), + ('crs_name', 'WGS 84'), + ('geog_citation', 'WGS 84'), + ('datum_code', 6326), + ('angular_units', 'degree'), + ('semi_major_axis', 6378137.0), + ('inv_flattening', 298.257223563), ] @@ -81,16 +84,28 @@ def test_deprecated_cases_cover_all_attrs(): ) +def test_deprecated_cases_has_no_duplicates(): + """Length-equality guard so a duplicate row in ``_DEPRECATED_CASES`` + cannot be silently absorbed by the set comparison in + ``test_deprecated_cases_cover_all_attrs``.""" + assert len(_DEPRECATED_CASES) == len(_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS), ( + f"length mismatch: _DEPRECATED_CASES has {len(_DEPRECATED_CASES)} " + f"rows but _DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS has " + f"{len(_DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS)} entries. Likely cause: " + f"a duplicate attr row in _DEPRECATED_CASES." + ) + + @pytest.mark.parametrize( - 'attr,field,value', + 'attr,value', _DEPRECATED_CASES, ids=[c[0] for c in _DEPRECATED_CASES], ) -def test_warns_on_emission(attr, field, value): +def test_warns_on_emission(attr, 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}) + info = _geo_info_with(**{attr: value}) with warnings.catch_warnings(record=True) as caught: warnings.simplefilter('always') @@ -109,18 +124,18 @@ def test_warns_on_emission(attr, field, value): @pytest.mark.parametrize( - 'attr,field,value', + 'attr,value', _DEPRECATED_CASES, ids=[c[0] for c in _DEPRECATED_CASES], ) -def test_emission_still_present(attr, field, value): +def test_emission_still_present(attr, 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}) + info = _geo_info_with(**{attr: value}) with warnings.catch_warnings(): warnings.simplefilter('ignore', DeprecationWarning) From 0b5fbef70e2c85a757db4ddb1f813109c930c02e Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 21:04:28 -0700 Subject: [PATCH 3/4] 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. --- xrspatial/geotiff/_attrs.py | 57 ++++++++++++++++++- ...est_attrs_pr7_deprecate_geographic_1984.py | 38 +++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index 38800ed32..6c22c0120 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -209,6 +209,55 @@ def _deprecated_geographic_geokey_warning(name: str) -> str: ) +def _stacklevel_to_external_caller() -> int: + """Return a ``stacklevel`` that points the warning at the first frame + outside :mod:`xrspatial.geotiff`. + + A fixed ``stacklevel`` is brittle here because the call chain to + ``warnings.warn`` differs by backend: + + * numpy path: ``open_geotiff`` -> emit helper -> ``warn`` (3 frames). + * dask path: ``open_geotiff`` -> ``read_geotiff_dask`` -> + ``_populate_attrs_from_geo_info`` -> emit helper -> ``warn`` (5 + frames). + * direct callers of ``read_geotiff_dask`` / ``read_to_array`` (used + internally and in tests) shorten the chain by one. + + Walk the stack from the warn-site outward and stop at the first + frame whose module is not ``xrspatial.geotiff*``. Returning a value + one greater than the index of that frame matches + :func:`warnings.warn` semantics (level 1 = the warn line itself). + + Today the warnings are :class:`DeprecationWarning`, which Python + silences by default for library code; the stacklevel mostly affects + the test suite. Get it right now so a future change to a louder + category (e.g. :class:`FutureWarning`) does not surface the warning + as if it came from ``_attrs.py``. + """ + import sys + + # Frame 0 is this function; frame 1 is the warn-site (the caller of + # this helper). Start the search at frame 1 so the returned level + # maps directly to the ``stacklevel`` argument passed to + # ``warnings.warn`` inside the warn-site. + frame = sys._getframe(1) + level = 1 + while frame is not None: + mod = frame.f_globals.get('__name__', '') + is_internal = ( + mod == 'xrspatial.geotiff' + or (mod.startswith('xrspatial.geotiff.') + and not mod.startswith('xrspatial.geotiff.tests')) + ) + if not is_internal: + return level + frame = frame.f_back + level += 1 + # Fell off the top of the stack without finding an external caller; + # fall back to a value that at least skips the warn-site itself. + return 2 + + def _emit_deprecated_geographic_geokey(attrs: dict, name: str, value) -> None: """Emit a deprecated geographic-GeoKey attr with a DeprecationWarning. @@ -216,11 +265,17 @@ def _emit_deprecated_geographic_geokey(attrs: dict, name: str, value) -> None: 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``. + + The ``stacklevel`` is computed by walking past every + ``xrspatial.geotiff*`` frame so the warning is attributed to the + user's call site (e.g. ``open_geotiff(...)``) rather than to one of + the internal read paths. A fixed level (e.g. ``stacklevel=2``) + would only be correct for one of the backend dispatch paths. """ warnings.warn( _deprecated_geographic_geokey_warning(name), DeprecationWarning, - stacklevel=2, + stacklevel=_stacklevel_to_external_caller(), ) attrs[name] = value diff --git a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py index d792faded..b83f5aee1 100644 --- a/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py +++ b/xrspatial/geotiff/tests/test_attrs_pr7_deprecate_geographic_1984.py @@ -177,3 +177,41 @@ def test_warning_message_format(): assert "deprecated" in msg assert "round-trip" in msg assert "#1984" in msg + + +def test_warning_stacklevel_points_at_caller_file(): + """The ``DeprecationWarning`` filename should land on the caller's + file, not on ``_attrs.py``. + + The emission helper computes ``stacklevel`` by walking past every + ``xrspatial.geotiff*`` frame, so the warning reports the first + external frame as its origin. The test file is outside that + package, so ``w.filename`` should match ``__file__``. If a future + refactor reintroduces a fixed ``stacklevel`` that is too small, + the warning will be reattributed to one of the internal modules + and this assertion will fail. + + Today the warning category is :class:`DeprecationWarning`, which + Python silences by default for library code, so the stacklevel + mostly affects test output. The pin lives here so a later switch + to :class:`FutureWarning` does not regress the attribution + silently. + """ + info = _geo_info_with(crs_name='WGS 84') + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + attrs: dict = {} + _populate_attrs_from_geo_info(attrs, info) + + matches = [w for w in caught + if issubclass(w.category, DeprecationWarning)] + assert len(matches) == 1, [ + (w.category.__name__, str(w.message)) for w in caught + ] + assert matches[0].filename == __file__, ( + f"warning filename {matches[0].filename!r} (line " + f"{matches[0].lineno}) does not match the test file " + f"{__file__!r}; the stacklevel walk did not exit the " + f"xrspatial.geotiff package." + ) From caf096756b0937496d202c414147a3aaa7c4dda1 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Sun, 17 May 2026 21:06:54 -0700 Subject: [PATCH 4/4] 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. --- docs/source/user_guide/attrs_contract.rst | 44 +++++++ xrspatial/geotiff/_attrs.py | 139 +++++++++++++--------- 2 files changed, 127 insertions(+), 56 deletions(-) diff --git a/docs/source/user_guide/attrs_contract.rst b/docs/source/user_guide/attrs_contract.rst index c1a04ec11..21a2a2b13 100644 --- a/docs/source/user_guide/attrs_contract.rst +++ b/docs/source/user_guide/attrs_contract.rst @@ -223,6 +223,50 @@ present on the original file may be absent after write→read if the canonical CRS does not carry enough information to rebuild it. +Deprecated GeoKey attrs (issue #1984) +===================================== + +The following attrs are still populated on read for one release +cycle but each emission fires a ``DeprecationWarning``. The writer's +``build_geo_tags`` only emits the primary CRS GeoKey and citation for +each axis (geographic, projected, vertical), so the secondary GeoKeys +these attrs derive from are never written and the values do not +survive a write→read round-trip. Migrate to ``crs`` / ``crs_wkt`` and +derive any needed value with :mod:`pyproj`. + +Geographic-CRS GeoKey attrs: ``crs_name``, ``geog_citation``, +``datum_code``, ``angular_units``, ``semi_major_axis``, +``inv_flattening``. + +Projected-CRS GeoKey attrs: ``linear_units``, ``projection_code``. + +Vertical-CRS GeoKey attrs: ``vertical_crs``, ``vertical_citation``, +``vertical_units``. + +Migration recipe:: + + from pyproj import CRS + crs = CRS.from_wkt(attrs['crs_wkt']) # or CRS.from_epsg(attrs['crs']) + + # Geographic + crs.name # crs_name + crs.datum.to_epsg() # datum_code + crs.ellipsoid.semi_major_metre # semi_major_axis + crs.ellipsoid.inverse_flattening # inv_flattening + # geog_citation / angular_units: best-effort derive from + # ``crs`` / ``crs.axis_info``; the original GeoKey citation text + # is not generally recoverable. + + # Projected + crs.coordinate_system.axis_list[0].unit_name # linear_units + crs.to_epsg() # projection_code + + # Vertical + crs.sub_crs_list[-1].to_epsg() # vertical_crs + crs.sub_crs_list[-1].name # vertical_citation + crs.sub_crs_list[-1].axis_info[0].unit_name # vertical_units + + Versioning ========== diff --git a/xrspatial/geotiff/_attrs.py b/xrspatial/geotiff/_attrs.py index 6c22c0120..908863163 100644 --- a/xrspatial/geotiff/_attrs.py +++ b/xrspatial/geotiff/_attrs.py @@ -195,17 +195,46 @@ ) +# Per-category reason clauses spliced into the deprecation warning by +# :func:`_emit_deprecated_geokey_attr`. Kept here so the wording stays +# in lockstep across the three GeoKey-axis tiers (geographic, projected, +# vertical) and so the test suite can match the canonical strings +# verbatim. +_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS = ( + "the writer cannot reconstruct it from the canonical CRS" +) +_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS = ( + "the writer cannot reconstruct vertical-CRS GeoKeys" +) + + +def _deprecated_geokey_warning(name: str, *, reason: str) -> str: + """Warning text for a deprecated GeoKey-derived attr. + + ``reason`` is the per-category clause that explains why the value + will not round-trip; the rest of the message is fixed so callers + only have to keep track of the short reason string. The wording is + pinned by ``test_warning_message_format`` (geographic tier) and by + sibling tests for the projected / vertical tiers, so any tweak + here needs to land alongside an update to those tests. + """ + return ( + f"xrspatial.geotiff: attrs[{name!r}] is deprecated; {reason} " + f"so it will not round-trip. It will be removed in a future " + f"release. See issue #1984." + ) + + 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. + Thin shim over :func:`_deprecated_geokey_warning` that fixes the + reason clause to the geographic-tier wording. Retained so existing + callers (notably the unit tests that pin the canonical wording) + keep working unchanged. """ - 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." + return _deprecated_geokey_warning( + name, reason=_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS, ) @@ -258,28 +287,51 @@ def _stacklevel_to_external_caller() -> int: return 2 -def _emit_deprecated_geographic_geokey(attrs: dict, name: str, value) -> None: - """Emit a deprecated geographic-GeoKey attr with a DeprecationWarning. +def _emit_deprecated_geokey_attr(attrs: dict, name: str, value, + *, reason: str) -> None: + """Emit a deprecated GeoKey-derived attr with a ``DeprecationWarning``. + + Generic helper shared by the geographic, projected, and vertical + deprecation tiers (issue #1984 PR 7). ``reason`` is the per-category + clause that explains why the value will not round-trip; it is + spliced into the warning text by :func:`_deprecated_geokey_warning`. + Use :data:`_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS` for the + geographic and projected tiers (both lose the value because the + writer cannot reconstruct it from the canonical CRS) and + :data:`_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS` for the vertical + tier (the writer skips the entire vertical GeoKey block). - 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``. + Sets ``attrs[name] = value`` after the warning so the read-side + emission stays alive for one release cycle and external callers + have time to migrate to ``crs`` / ``crs_wkt``. The ``stacklevel`` is computed by walking past every ``xrspatial.geotiff*`` frame so the warning is attributed to the user's call site (e.g. ``open_geotiff(...)``) rather than to one of - the internal read paths. A fixed level (e.g. ``stacklevel=2``) - would only be correct for one of the backend dispatch paths. + the internal read paths. """ warnings.warn( - _deprecated_geographic_geokey_warning(name), + _deprecated_geokey_warning(name, reason=reason), DeprecationWarning, stacklevel=_stacklevel_to_external_caller(), ) attrs[name] = value +def _emit_deprecated_geographic_geokey(attrs: dict, name: str, value) -> None: + """Geographic-tier wrapper around :func:`_emit_deprecated_geokey_attr`. + + Kept as a thin shim so the geographic emission sites in + :func:`_populate_attrs_from_geo_info` stay readable and so a future + diff touching only the geographic tier does not need to repeat the + ``reason=`` clause at every call site. + """ + _emit_deprecated_geokey_attr( + attrs, name, value, + reason=_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS, + ) + + 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). @@ -406,15 +458,10 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None _emit_deprecated_geographic_geokey( attrs, 'angular_units', geo_info.angular_units) if geo_info.linear_units is not None: - warnings.warn( - "xrspatial.geotiff: attrs['linear_units'] is deprecated; " - "the writer cannot reconstruct it from the canonical CRS " - "so it will not round-trip. It will be removed in a future " - "release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'linear_units', geo_info.linear_units, + reason=_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS, ) - attrs['linear_units'] = geo_info.linear_units if geo_info.semi_major_axis is not None: _emit_deprecated_geographic_geokey( attrs, 'semi_major_axis', geo_info.semi_major_axis) @@ -422,45 +469,25 @@ def _populate_attrs_from_geo_info(attrs: dict, geo_info, *, window=None) -> None _emit_deprecated_geographic_geokey( attrs, 'inv_flattening', geo_info.inv_flattening) if geo_info.projection_code is not None: - warnings.warn( - "xrspatial.geotiff: attrs['projection_code'] is deprecated; " - "the writer cannot reconstruct it from the canonical CRS " - "so it will not round-trip. It will be removed in a future " - "release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'projection_code', geo_info.projection_code, + reason=_GEOKEY_DEPRECATION_REASON_HORIZONTAL_CRS, ) - attrs['projection_code'] = geo_info.projection_code if geo_info.vertical_epsg is not None: - warnings.warn( - "xrspatial.geotiff: attrs['vertical_crs'] is deprecated; " - "the writer cannot reconstruct vertical-CRS GeoKeys so it " - "will not round-trip. It will be removed in a future " - "release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'vertical_crs', geo_info.vertical_epsg, + reason=_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS, ) - attrs['vertical_crs'] = geo_info.vertical_epsg if geo_info.vertical_citation is not None: - warnings.warn( - "xrspatial.geotiff: attrs['vertical_citation'] is deprecated; " - "the writer cannot reconstruct vertical-CRS GeoKeys so it " - "will not round-trip. It will be removed in a future " - "release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'vertical_citation', geo_info.vertical_citation, + reason=_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS, ) - attrs['vertical_citation'] = geo_info.vertical_citation if geo_info.vertical_units is not None: - warnings.warn( - "xrspatial.geotiff: attrs['vertical_units'] is deprecated; " - "the writer cannot reconstruct vertical-CRS GeoKeys so it " - "will not round-trip. It will be removed in a future " - "release. See issue #1984.", - DeprecationWarning, - stacklevel=2, + _emit_deprecated_geokey_attr( + attrs, 'vertical_units', geo_info.vertical_units, + reason=_GEOKEY_DEPRECATION_REASON_VERTICAL_CRS, ) - attrs['vertical_units'] = geo_info.vertical_units if geo_info.gdal_metadata is not None: attrs['gdal_metadata'] = geo_info.gdal_metadata