Skip to content

Preserve caller coords/res/crs in generate_terrain (#3474)#3475

Merged
brendancol merged 2 commits into
mainfrom
issue-3474
Jun 23, 2026
Merged

Preserve caller coords/res/crs in generate_terrain (#3474)#3475
brendancol merged 2 commits into
mainfrom
issue-3474

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3474

generate_terrain() was building fresh y/x coordinates from
x_range/y_range (default (0, 500)) and setting attrs to only
{'res': (dx, dy)}, so a georeferenced caller lost its coords, res, crs,
and units. This was easiest to hit through xarr.xrs.generate_terrain().

Now, when the caller's array carries its own y/x coords, the output reuses
them and copies the caller's attrs (crs, res, units, ...). x_range/y_range
still 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 res attr, res is derived from the coord
spacing (which equals the old (dx, dy) for the synthetic case, so the
bare-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:

  • caller coords / res / crs / units preserved (numpy + accessor)
  • dask: chunks, coords, attrs preserved
  • bare template unchanged (synthetic grid, computed res, no crs)
  • coords-without-res derives res from spacing
  • full test_terrain.py suite passes (43 passed); cupy paths covered by guarded tests

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 computes dx/dy for the coord linspace, then the res fallback 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 exactly dx, so the bare-template res derived from coord spacing equals the old (dx, dy). The width==1/height==1 fallback to x_range/y_range span also matches the old (x1-x0)/1.
  • The erode=True path is covered: erode() rebuilds the output with coords=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), and res is 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 brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@brendancol brendancol merged commit b606aef into main Jun 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

generate_terrain() drops caller's coords, res, and crs

1 participant