Skip to content

aspect: planar dask backends report float32 dtype matching computed data#2741

Merged
brendancol merged 4 commits into
mainfrom
deep-sweep-metadata-aspect-2026-05-29
May 30, 2026
Merged

aspect: planar dask backends report float32 dtype matching computed data#2741
brendancol merged 4 commits into
mainfrom
deep-sweep-metadata-aspect-2026-05-29

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2682

What this fixes

The planar dask backends of aspect() advertised a lazy dtype of
float64, but the array they actually computed was float32. The numpy
and cupy backends already reported float32, so the dtype you saw
depended on which backend was active, and it silently changed from
float64 to float32 once .compute() ran.

Root cause: _run_dask_numpy and _run_dask_cupy called map_overlap
with a default-dtype meta (np.array(()) / cupy.array(())), which
defaults to float64. The chunk functions _cpu / _run_cupy return
float32. The fix passes an explicit float32 dtype to the meta, matching
what the geodesic dask paths already do.

northness() and eastness() derive from aspect(), so they inherit
the corrected dtype.

Backend coverage

  • numpy: already float32, unchanged
  • cupy: already float32, unchanged
  • dask+numpy: was lazy-float64 / computed-float32, now float32 both
  • dask+cupy: was lazy-float64 / computed-float32, now float32 both

Test plan

  • New test_dask_numpy_advertised_dtype_matches_computed asserts lazy
    and computed dtype are both float32
  • New test_dask_cupy_advertised_dtype_matches_computed covers the
    GPU dask path
  • Full test_aspect.py suite passes (66 passed, cupy + dask+cupy
    included on a CUDA host)

Note

slope.py and curvature.py use the same default-dtype meta pattern on
their planar dask paths and likely have the same inconsistency. Out of
scope for this PR (metadata sweep was scoped to aspect).

The planar dask backends called map_overlap with a default-dtype meta
(np.array(()) / cupy.array(())), so the lazy DataArray advertised float64
while the chunk functions return float32. numpy and cupy backends already
report float32. Pass an explicit float32 dtype to the meta so the
advertised dtype matches the computed data across all four backends.

The geodesic dask paths already set dtype=np.float32; only the planar
paths were affected.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 30, 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: aspect planar dask backends report float32 dtype matching computed data

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None. One optional coverage gap below.

Nits (optional improvements)

  • The two new tests only cover the default boundary='nan' path. The
    other boundary modes run through _pad_array and then hit the same
    chunk functions, so they return float32 too, but a parametrize over
    boundary would pin that down. Low value, skip if you prefer.

What looks good

  • The fix is minimal and correct. _cpu / _run_cupy already cast to
    float32 and return it, so declaring float32 in the meta is the right
    call, and it matches what the geodesic dask paths in this same file
    already do.
  • numpy and cupy already reported float32. This brings the two planar
    dask paths in line, so all four backends now agree on both the lazy
    and the computed dtype.
  • Both dask paths get the same treatment.
  • The tests check both the lazy advertised dtype and the dtype after
    compute, which is the actual bug in #2682: the two disagreed.
  • The computation itself is untouched. No change to NaN handling, depth,
    or boundary forwarding.

Checklist

  • Algorithm: unchanged, dtype-only fix
  • Backend consistency: float32 verified on numpy, cupy, dask+numpy, dask+cupy
  • NaN handling: unchanged
  • Edge cases: dtype consistency covered; boundary-mode dtype not parametrized (low risk)
  • Dask chunk boundaries: unchanged, depth=(1,1)
  • No premature materialization or copies: confirmed
  • Benchmark: not needed, no perf change
  • README feature matrix: not applicable, no API change
  • Docstrings: unchanged

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 (after 7c9017a)

The one nit from the previous pass is addressed: test_dask_numpy_advertised_dtype_matches_computed is now parametrized over all four boundary modes (nan, nearest, reflect, wrap), so the _pad_array path is covered for dtype too. All five dtype tests pass.

No new findings. Nothing blocking.

@brendancol brendancol merged commit 4225d8e into main May 30, 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.

aspect() planar dask backends report float64 dtype but compute float32

1 participant