Preserve caller coords/res/crs in generate_terrain (#3474)#3475
Merged
Conversation
brendancol
commented
Jun 23, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Preserve caller coords/res/crs in generate_terrain (#3474)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
xrspatial/tests/test_terrain.py: there's no explicit dask+cupy parity test for the preservation path. The wrapper code is backend-agnostic so the risk is low, and the test would need a GPU runner, so this is fine to leave. Noting it for the record.
Nits (optional improvements)
xrspatial/terrain.py:711-720: the synthetic branch computesdx/dyfor the coordlinspace, then theresfallback re-derives spacing from the coords. That's intentional (single source of truth: the actual coord spacing) and produces the same(dx, dy)as before, so no change needed — just flagging the small redundancy.
What looks good
- Backward compatibility is exact.
linspace(x0+dx/2, x1-dx/2, width)has spacing exactlydx, so the bare-templateresderived from coord spacing equals the old(dx, dy). Thewidth==1/height==1fallback tox_range/y_rangespan also matches the old(x1-x0)/1. - The
erode=Truepath is covered:erode()rebuilds the output withcoords=agg.coords, attrs=agg.attrs(erosion.py:496-497), so crs/res/coords survive erosion too. - Fix lives in the backend-agnostic public wrapper, so all four backends get the same treatment. Dask data chunks stay chunked, and the dask test asserts
terrain.data.chunks == src.data.chunks. - Caller attrs are copied with
dict(agg.attrs)(shallow copy is fine for the str/tuple values here), andresis only synthesized when the caller didn't supply one.
Checklist
- Algorithm matches reference/paper (noise unchanged; only coord/attr wrapping)
- All implemented backends produce consistent results
- NaN handling unchanged
- Edge cases covered by tests (bare template, coords-without-res)
- Dask chunk boundaries handled correctly (chunks asserted preserved)
- No premature materialization or unnecessary copies
- Benchmark not needed (no perf-critical path changed)
- README feature matrix not applicable (no new function)
- Docstrings present and accurate (agg param documents the new behavior)
brendancol
commented
Jun 23, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review (after c0ede6b)
The one suggestion from the first pass is addressed: added
test_generate_terrain_dask_cupy_preserves_coords_attrs, double-guarded by
@cuda_and_cupy_available and @dask_array_available.
This box happens to have CUDA + cupy, so all four backends' preservation tests
ran for real and pass:
- test_generate_terrain_preserves_caller_coords_and_attrs (numpy)
- test_generate_terrain_accessor_preserves_georeference
- test_generate_terrain_dask_preserves_chunks_coords_attrs (dask+numpy)
- test_generate_terrain_cupy_preserves_coords_attrs (cupy)
- test_generate_terrain_dask_cupy_preserves_coords_attrs (dask+cupy)
Full test_terrain.py: 44 passed. No outstanding blockers, suggestions, or
nits. The earlier nit (synthetic-branch res re-derived from coord spacing) was
intentional and dismissed.
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
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.
Closes #3474
generate_terrain()was building freshy/xcoordinates fromx_range/y_range(default(0, 500)) and settingattrsto only{'res': (dx, dy)}, so a georeferenced caller lost its coords,res,crs,and
units. This was easiest to hit throughxarr.xrs.generate_terrain().Now, when the caller's array carries its own
y/xcoords, the output reusesthem and copies the caller's attrs (
crs,res,units, ...).x_range/y_rangestill drive the noise field. A bare template with no coords keeps the old
behavior: a synthetic grid plus a computed
res.When the caller has coords but no
resattr,resis derived from the coordspacing (which equals the old
(dx, dy)for the synthetic case, so thebare-template output is unchanged).
Backends: numpy, cupy, dask+numpy, dask+cupy. Dask data chunks were already
preserved through
map_blocks; this restores the coords and attrs on top.Test plan: