geotiff: follow-up review nits from #1984 PRs 2 and 5#2012
Merged
Conversation
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.
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.
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
Picks up two review nits from #2001 and #2002 that did not make it into the merged commits. The three sibling nits from #2000, #2003, and #2004 already landed.
Changes
docs/source/user_guide/attrs_contract.rst(#2001 follow-up):transformrow: corrected the tuple layout from(origin_x, pixel_width, 0, origin_y, 0, pixel_height)to the rasterio Affine ordering(pixel_width, 0.0, origin_x, 0.0, pixel_height, origin_y)that_transform_tuple_from_pixel_geometryactually emits. The page was the only remaining site advertising the wrong layout (the module docstring in_attrs.pywas already corrected).crs_wktrow: relaxed "Always present" and "WKT2 string" to reflect that the dialect depends on the source path.nodatarow: documented the read-side precedence chain (nodata, thennodatavals, then_FillValue).colormap_rgba/cmaprows: noted thePhotometric == 3gate.xrspatial/geotiff/tests/test_attrs_contract_aliases_1984.py(#2002 follow-up):_SENTINEL: one-line comment explaining the value is arbitrary.Test plan
pytest xrspatial/geotiff/tests/test_attrs_contract_aliases_1984.py -x -q— 5 passed.