Skip to content

resample: refresh transform to match actual coord orientation (#2571)#2587

Merged
brendancol merged 2 commits into
mainfrom
issue-2571
May 29, 2026
Merged

resample: refresh transform to match actual coord orientation (#2571)#2587
brendancol merged 2 commits into
mainfrom
issue-2571

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2571.

Summary

  • resample() was always writing a north-up rasterio transform
    (+x, -y, top-left origin), even when the input had ascending y
    or descending x coords. The refreshed attrs['transform'] then no
    longer matched the coords on the same DataArray, so downstream
    consumers that georeference via transform would see a flipped
    raster.
  • Use the signed px / py from _new_coords and anchor the
    origin at *_edge_start (the leading edge on the side of
    vals[0]). This matches rasterio's (col=0, row=0) -> first array pixel convention for every coord orientation.

Backend coverage

The transform refresh runs in pure Python on the metadata after the
backend dispatch, so the fix applies to numpy, cupy, dask+numpy, and
dask+cupy identically. No backend-specific code changed.

Test plan

  • pytest xrspatial/tests/test_resample.py::TestMetadataPropagation
    -- 19 passed, includes the 4 new orientation parametrizations.
  • pytest xrspatial/tests/test_resample.py -- 182 passed.

The transform refresh block at the end of resample() was hard-coding
a north-up rasterio tuple (positive x scale, negative y scale, origin
at the top-left edge). For arrays with ascending y or descending x
coords that produced an attrs['transform'] that did not match the
coords on the same DataArray, so downstream code that georeferenced
via attrs['transform'] would get a flipped raster.

Use the signed px / py returned by _new_coords (positive when the
coord ascends along the axis, negative when it descends) and anchor
the origin at *_edge_start, which is already the leading edge of the
first row / column on the side of vals[0]. That matches rasterio's
`(col=0, row=0) -> first array pixel` convention for every coord
orientation.

Tests cover all four orientation combinations (x asc/desc x y
asc/desc) and assert the refreshed transform round-trips against the
output coords.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 28, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: resample transform refresh (#2571)

Blockers

None.

Suggestions

None.

Nits

  • xrspatial/tests/test_resample.py:163 -- the _raster_with_orientation
    docstring says "upper-left edge of pixel [0, 0]", but for ascending
    y the first pixel sits at the geographic lower-left. The
    transform is still right; the wording just borrows rasterio's
    "upper-left" convention where "leading-edge corner of pixel [0, 0]"
    would be more honest.
  • xrspatial/tests/test_resample.py:191 -- assert b == 0.0 and d == 0.0
    packs two facts into one assertion. Splitting them tells you which
    off-diagonal drifted when it fails.

What looks good

  • The math holds up. px / py out of _new_coords are already
    signed, and *_edge_start is the leading edge on the vals[0]
    side, so transform * (col, row) lands where the output coords
    say it should regardless of orientation.
  • The pre-existing test_transform_refreshed_on_downsample is
    untouched and still passes, which is a useful regression anchor.
  • Metadata-only change, so the fix covers numpy, cupy, dask+numpy,
    and dask+cupy without any per-backend code.
  • The parametrized test sweeps all four x asc/desc x y asc/desc
    combos and round-trips transform * (col+0.5, row+0.5) against
    the output pixel centers.

Checklist

  • Algorithm matches the rasterio Affine convention
  • All backends consistent (metadata-only)
  • NaN handling unchanged
  • Orientation edge cases covered
  • Dask chunks N/A
  • No premature materialization
  • Benchmark not needed
  • README feature matrix not applicable
  • Docstrings unchanged

- `_raster_with_orientation` docstring now says "leading-edge corner"
  rather than "upper-left edge", which only matches when both coords
  are north-up.
- Split `assert b == 0.0 and d == 0.0` into two asserts so a failure
  identifies which off-diagonal drifted.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review (#2571)

Both nits from the previous review are fixed in 34dc110:

  • _raster_with_orientation docstring now says "leading-edge corner"
    instead of "upper-left edge".
  • The combined off-diagonal assert is split into two.

No new findings on the follow-up pass. The 19
TestMetadataPropagation tests still pass locally.

@brendancol brendancol merged commit a347ab7 into main May 29, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

resample: attrs['transform'] is wrong for ascending-y or descending-x inputs

1 participant