geotiff: add attrs compatibility-alias locking test (#1984)#2002
Merged
Conversation
Locks the contract for the rioxarray-style ``attrs['nodatavals']`` and CF-style ``attrs['_FillValue']`` aliases in one place so future drift surfaces here rather than in scattered regression files. Five cases: - ``test_read_uses_nodatavals_when_nodata_absent``: rioxarray-style tuple round-trips through ``to_geotiff``. - ``test_read_uses_fill_value_when_nodata_absent``: CF ``_FillValue`` round-trips through ``to_geotiff``. - ``test_canonical_nodata_wins_over_aliases``: ``attrs['nodata']`` beats both aliases, asserted at both the resolver and the round-trip layer. - ``test_write_does_not_emit_aliases_when_canonical_present``: ``open_geotiff`` emits only ``nodata`` after a write, not the aliases. - ``test_nan_in_nodatavals_is_skipped``: NaN entries in the rioxarray tuple are skipped, not treated as a concrete sentinel. Tests only. No production code changes. PR 5 of 7 for #1984.
1 task
Contributor
Author
PR Review: geotiff: add attrs compatibility-alias locking testBlockers (must fix before merge)None. Suggestions (should fix, not blocking)None. Nits (optional improvements)
What looks good
Checklist
|
brendancol
added a commit
that referenced
this pull request
May 17, 2026
Addresses #2002 review nit. The value itself does not matter; only that it is float-castable and distinct from any data value the tests use.
2 tasks
brendancol
added a commit
that referenced
this pull request
May 18, 2026
* geotiff: add attrs canonical-tier locking test (#1984) PR 4 of 7 from the attrs-contract plan in #1984. Adds test_attrs_contract_canonical_1984.py: a single fixture that exercises every canonical attr (crs, crs_wkt, transform, nodata, raster_type, extra_tags, gdal_metadata, gdal_metadata_xml, x_resolution / y_resolution / resolution_unit, _xrspatial_geotiff_contract), round-trips it through to_geotiff -> open_geotiff, and asserts presence + value per key. Sibling locking tests already cover the other tiers: aliases (#2002), pass-through (#2004), and per-backend stamping of the contract version (#2003). The canonical assertion list is what #1985 (parity matrix) and #1986 (round-trip invariants) will import. No production-code changes. * geotiff: address review on attrs canonical-tier locking test Addresses self-review on PR #2009: * Add per-backend canonical-key presence parametrisation (eager-numpy, dask-numpy, gpu, dask-gpu). The 7-PR plan in #1984 called for "one fixture per backend" coverage; previously only the eager read path was exercised. * Comment ``_CANONICAL_KEYS`` to explain the deliberate omission of ``raster_type`` (the implicit 'area' default is encoded as absence, so the constant cannot express it; the two dedicated raster_type tests handle both branches). * Drop ``AREA_OR_POINT: 'Area'`` from the shared ``_GDAL_META`` fixture so the point-branch test does not inherit an inconsistent GDAL_METADATA entry. * Relax the crs_wkt substring check to a regex covering 'WGS 84', 'WGS_1984', and 'WGS-84' so the assertion survives PROJ-version variation across CI platforms. * Note in a comment that ``gdal_metadata['TIFFTAG_SOFTWARE']`` and the raw Software TIFF tag (305) in ``extra_tags`` are independent channels the writer does not synchronise.
2 tasks
brendancol
added a commit
that referenced
this pull request
May 18, 2026
Two nits from the post-merge reviews on #2001 and #2002 that did not make it into the merged commits. #2001 user-guide page (``docs/source/user_guide/attrs_contract.rst``): - ``transform``: corrected layout from ``(origin_x, pixel_width, 0, origin_y, 0, pixel_height)`` (GDAL ordering) to the rasterio Affine ordering ``(pixel_width, 0.0, origin_x, 0.0, pixel_height, origin_y)`` that ``_transform_tuple_from_pixel_geometry`` actually emits. - ``crs_wkt``: relaxed "Always present" and "WKT2 string" to reflect that the dialect depends on the source path (pyproj synthesis is WKT2; VRT SRS-tag passthrough carries whatever dialect was on disk). - ``nodata``: documented the read-side precedence chain (``nodata``, then ``nodatavals``, then ``_FillValue``) as codified in ``_resolve_nodata_attr``. - ``colormap_rgba`` / ``cmap``: noted the ``Photometric == 3`` gate that the reader applies before emitting either attr. #2002 alias locking test (``xrspatial/geotiff/tests/test_attrs_contract_aliases_1984.py``): - ``_SENTINEL``: added a one-line comment that the value just needs to be float-castable and distinct from data; the resolver and round-trip assertions are what the tests pin. Coordinates with the canonical-tier docstring already corrected on ``_attrs.py`` in main.
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.
PR 5 of 7 for issue #1984.
Summary
Adds
xrspatial/geotiff/tests/test_attrs_contract_aliases_1984.py, a single locking test that pins the compatibility alias tier of the attrs contract so future drift surfaces in one place. No production code changes; tests only.The contract being pinned:
attrs['nodatavals'](rioxarray) andattrs['_FillValue'](CF) on a foreign DataArray, falling back to them whenattrs['nodata']is absent.attrs['nodata']is present; after a round-trip onlynodatashould appear.Finer-grained cases continue to live in
test_nodata_attr_aliases_1582.py. This file is intentionally contract-shaped and does not duplicate those cases.Tests added
test_read_uses_nodatavals_when_nodata_absent: pins that a foreign DataArray carrying only the rioxarray tuple survives the writer.test_read_uses_fill_value_when_nodata_absent: same for CF_FillValue.test_canonical_nodata_wins_over_aliases: when all three keys are present and disagree,attrs['nodata']wins. Asserted at both the_resolve_nodata_attrchokepoint and the round-trip layer.test_write_does_not_emit_aliases_when_canonical_present: after writing a DataArray that has onlynodataset, the reader returns attrs containingnodataand neither alias.test_nan_in_nodatavals_is_skipped: NaN entries in a rioxarray tuple are skipped, not treated as a concrete sentinel. Pure-NaN tuples resolve toNone; mixed tuples return the first concrete numeric value.Bugs spotted
None. Reader paths in
_attrs._populate_attrs_from_geo_infoonly setattrs['nodata']; neithernodatavalsnor_FillValueis written into the output attrs after a round-trip, so all five assertions pass on the current code. No xfails needed.Test plan
pytest xrspatial/geotiff/tests/test_attrs_contract_aliases_1984.py -x -q-- 5 passed.pytest xrspatial/geotiff/tests -x -qsanity check. Two pre-existing failures observed onmain(test_predictor2_big_endian_gpu_1517.py-- missingread_to_arrayattribute;test_size_param_validation_gpu_vrt_1776.py::test_tile_size_positive_works-- unrelated_validation.pyValueError). Both reproduce without this PR's changes, so unrelated.Closes-step: 5 of 7 toward #1984.