Skip to content

Preserve input float dtype in focal.mean() across all backends (#3214)#3221

Merged
brendancol merged 3 commits into
mainfrom
deep-sweep-accuracy-focal-2026-06-10-01
Jun 10, 2026
Merged

Preserve input float dtype in focal.mean() across all backends (#3214)#3221
brendancol merged 3 commits into
mainfrom
deep-sweep-accuracy-focal-2026-06-10-01

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3214

  • mean() now uses _promote_float on every backend: float64 stays float64, float32 stays float32, integers promote to float32. Previously numpy/dask+numpy always returned float64 and cupy/dask+cupy always returned float32, so the output dtype depended on the backend.
  • Drops the hardcoded cupy.float32 casts in _mean_cupy / _mean_dask_cupy. float64 rasters keep their precision on GPU; with values near 1e7 the old float32 path's error (0.58) was larger than the spread of the true focal means (0.42).
  • excludes values are cast to the working dtype before matching, so exclusion behaves the same on CPU and GPU for float32 rasters.

This is the same contract apply() and focal_stats() got in #2769 and convolve_2d() got in #1096; mean() was the holdout.

Backend coverage: numpy, cupy, dask+numpy, dask+cupy. All four ran locally on a CUDA host.

The diff also updates one row of .claude/sweep-accuracy-state.csv (accuracy-sweep bookkeeping for the focal module).

Test plan:

  • New dtype tests: float64 preserved, float32 preserved, int promotes to float32, parametrized across backends
  • New GPU large-offset parity test (numpy vs cupy at offset 1e7, rtol 1e-12)
  • New excludes-dtype matching test
  • Full xrspatial/tests/test_focal.py: 245 passed on a GPU host
  • test_accessor.py + test_emerging_hotspots.py: 113 passed

mean() cast to float64 on numpy/dask+numpy and to float32 on
cupy/dask+cupy, so the output dtype depended on the backend and
float64 rasters lost precision on GPU. Use _promote_float (the
same contract as apply/focal_stats from #2769) and cast excludes
to the working dtype so value matching agrees across backends.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 10, 2026

@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 input float dtype in focal.mean() across all backends (#3214)

Blockers (must fix before merge)

None found.

Suggestions (should fix, not blocking)

  • test_mean_excludes_match_in_working_dtype_3214 (xrspatial/tests/test_focal.py) only runs on numpy. The cupy path has its own excludes cast (cupy.asarray(excludes, dtype=fdtype) in _mean_cupy, xrspatial/focal.py:262-264), so the float32 exclusion-match behavior should be asserted there too. Parametrizing the test over numpy and cupy covers it.
  • This PR changes integer-input behavior on the CPU backends: int rasters used to come back float64 and now come back float32, so int values above 2^24 are no longer exact in the output. That matches the apply()/focal_stats()/convolve_2d() contract, but the mean() docstring (xrspatial/focal.py, Returns section) says nothing about output dtype. One sentence stating the dtype contract would prevent surprise.

Nits (optional improvements)

  • The two F401 'cupy' imported but unused warnings at test_focal.py:1688 and 1704 predate this PR (from the #2730 tests) and are correctly left alone here.

What looks good

  • All four backends now share one dtype rule, applied at the dispatch boundary (mean()) and inside the chunk function (_mean_cupy), so the dask+cupy path is covered both lazily and per chunk.
  • Casting excludes to the working dtype closes a cross-backend mismatch that the float32 change would otherwise have introduced (CPU comparing float32 cells against float64 excludes).
  • The large-offset GPU test pins parity at rtol 1e-12 and asserts the focal-mean spread survives, which is the actual failure mode from the issue, not just a dtype check.
  • A time_mean benchmark already exists in benchmarks/benchmarks/focal.py, so no benchmark gap.

Checklist

  • Algorithm matches reference (no algorithm change; dtype handling only)
  • All implemented backends produce consistent results (verified on a CUDA host)
  • NaN handling is correct (NaN excludes survive the dtype cast; _equal_numpy uses isnan)
  • Edge cases are covered by tests (float64/float32/int inputs, large offset, excludes matching)
  • Dask chunk boundaries handled correctly (no depth changes; astype is lazy)
  • No premature materialization or unnecessary copies
  • Benchmark exists
  • README feature matrix unchanged (no backend support change)
  • Docstring does not state the output dtype contract (see suggestion above)

@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 (commit 83be5d1)

Both suggestions from the first pass are addressed:

  • test_mean_excludes_match_in_working_dtype_3214 is now parametrized over numpy and cupy and handles the device-array extraction with .get(). Verified both variants pass on a CUDA host.
  • The mean() Returns section now states the dtype contract (float preserved, int promotes to float32, matching apply and focal_stats).

No new findings. The pre-existing F401 nits remain out of scope.

Disposition of first-pass findings: 2 suggestions fixed, 1 nit dismissed (pre-existing, untouched by this PR), 0 blockers.

@brendancol brendancol merged commit f79833c into main Jun 10, 2026
7 checks passed
brendancol added a commit that referenced this pull request Jun 10, 2026
…ocal-2026-06-10-01

Conflicts:
- xrspatial/focal.py: main's #3221 (issue #3214, a duplicate of #3217)
  changed mean() to the _promote_float contract; kept main's GPU/dask
  cast lines, this branch keeps the typed map_overlap metas.
- xrspatial/tests/test_focal.py: aligned the 3217 mean dtype test with
  the _promote_float contract (float32 in -> float32 out).
- .claude/sweep-metadata-state.csv: restored LF line endings, kept
  main's geotiff row and this branch's focal row (notes updated).
brendancol added a commit that referenced this pull request Jun 11, 2026
Main's #3221 (issue #3214) landed the same float64-preservation fix
for the mean() GPU paths that this branch made for #3222, so the
merge takes main's focal.py wholesale. The branch's regression tests
survive: multi-pass GPU parity and dask+cupy large-offset parity are
not covered by #3221's tests, and the old rtol=1e-4 allowance on the
dask+gpu transfer test can tighten now that every backend computes
in float64.
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.

focal.mean() returns float32 on cupy backends and float64 on numpy, losing precision on large-offset rasters

1 participant