Skip to content

Promote 32/64-bit integer rasters to float64 in _promote_float (#3684) - #3764

Open
brendancol wants to merge 2 commits into
mainfrom
issue-3684
Open

brendancol wants to merge 2 commits into
mainfrom
issue-3684

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3684

_promote_float in xrspatial/convolution.py mapped every integer dtype to float32. float32 has a 24-bit mantissa, so int32/int64 rasters with values above ~16.7M lost their low bits before the kernel ran, and convolve_2d, convolution_2d, focal.mean, apply and focal_stats gave wrong answers with no warning. The repro from the issue (an int32 raster offset by 1e8 under a gradient kernel) returned 0 where the float64 reference gives 8.

The rule is now: floats keep their dtype; int8/uint8/int16/uint16 still go to float32, which holds them exactly; int32/uint32/int64/uint64 go to float64. That doubles working memory for wide-int input on the focal paths, and the memory-budget guards in apply / focal_stats already size themselves from _promote_float, so they pick that up automatically.

Docs touched: the mean and convolve_2d docstrings, the focal memory-budget comments, the note in focal.rst, and the data-types guide. The convolution_2d docstring examples lose their dtype=float32 tag because np.arange input is int64 and now comes out float64.

Tests: test_mean_int_promotes_to_float32_3214 becomes a parametrized promotion test over narrow and wide ints, and the three #3217 dtype tests compute their expectation through _promote_float instead of restating the old rule. New regression tests run the issue's repro on numpy, dask+numpy, cupy and dask+cupy and compare exactly (not allclose, which would hide the bug at 1e8). GPU tests ran locally.

Left alone: edge_detection._promote_wide_int from #3680 pre-casts wide ints to float64 before calling convolve_2d. It is redundant now but harmless, and removing it would widen this PR past the shared helper.

Test plan:

  • pytest xrspatial/tests/test_convolution.py xrspatial/tests/test_focal.py xrspatial/tests/test_edge_detection.py (699 passed, GPU included)
  • pytest xrspatial/tests/test_bilateral.py xrspatial/tests/test_emerging_hotspots.py xrspatial/tests/test_accessor.py xrspatial/tests/test_dask_laziness.py (200 passed)
  • CI

float32 cannot separate integers above 2**24, so convolve_2d, focal.mean,
apply and focal_stats returned wrong results for int32/int64 rasters with
large values. Narrow ints (8/16-bit) still go to float32, which holds them
exactly; wide ints now go to float64. Floats keep their dtype as before.

The contract tests from #3214 and #3217 pinned int32 -> float32; they now
assert the promotion rule through _promote_float itself, with separate
cases for narrow and wide ints. New regression tests check that an int32
raster offset by 1e8 matches the float64 reference exactly on numpy,
dask+numpy, cupy and dask+cupy.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Nov 6, 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: Promote 32/64-bit integer rasters to float64 in _promote_float (#3684)

Blockers (must fix before merge)

  • None.

Suggestions (should fix, not blocking)

  • xrspatial/edge_detection.py:34: the _promote_wide_int docstring still says "convolve_2d promotes integer inputs to float32", which this PR makes false. Keeping the helper is fine, but the docstring should say the helper is now redundant with _promote_float (or the helper should go in a follow-up). Leaving a wrong sentence next to the fix is how the next sweep re-files this.
  • xrspatial/tests/test_convolution.py:67-80 (test_convolution_2d_wide_int_exact_above_2_24_3684): the backend loop means the first failing backend hides the rest, and the test id does not say which backend failed. Parametrize over backend like the focal twin does.

Nits (optional improvements)

  • xrspatial/tests/test_focal.py (test_mean_wide_int_exact_above_2_24_3684): chunks=(2, 3) is passed for the numpy and cupy backends too, where create_test_raster ignores it. Harmless, just noise.
  • docs/source/user_guide/data_types.rst:12 still opens with "standardizes on float32 for output in most analytical functions". Still true for most of the library, but a reader who hits the new int32 example a page later may want a pointer back to that sentence.

What looks good

  • The memory-budget guards in apply and focal_stats compute itemsize from _promote_float, so they already charge 8 bytes/cell for wide ints; no second edit was needed.
  • The regression tests compare with assert_array_equal, not allclose. At 1e8 a relative tolerance of 1e-7 is 10, which would have passed the broken code.
  • The #3214 / #3217 contract tests now derive their expectation from the helper instead of restating the rule, so the next dtype change will not have to touch them.
  • hotspots is untouched; it pins float32 on purpose (#3223) and is documented as such.

Checklist

  • Algorithm matches reference/paper (exact float64 reference)
  • All implemented backends produce consistent results (numpy, dask+numpy, cupy, dask+cupy tested)
  • NaN handling is correct (unchanged)
  • Edge cases are covered by tests (narrow vs wide ints, bool rejected upstream)
  • Dask chunk boundaries handled correctly (unchanged; dask meta follows fdtype)
  • No premature materialization or unnecessary copies (the astype was already there; only the target dtype changed)
  • Benchmark exists or is not needed (convolution.py / focal.py benchmarks exist)
  • README feature matrix updated (not applicable)
  • Docstrings present and accurate

…ote the edge_detection pre-cast is redundant (#3684)

@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: follow-up after d124db6

All four findings from the first pass are addressed in d124db6:

  • edge_detection._promote_wide_int docstring no longer claims convolve_2d promotes to float32; it says the pre-cast is redundant since #3684 and why it stays.
  • test_convolution_2d_wide_int_exact_above_2_24_3684 is parametrized over the four backends, with the usual cupy skip.
  • Both wide-int regression tests carry a comment that chunks=(2, 3) only applies to the dask backends.
  • data_types.rst opening sentence now points at the focal/convolution exception for 32/64-bit ints.

Re-ran test_convolution.py, test_focal.py, test_edge_detection.py locally with GPU: 705 passed. Nothing new to flag.

This branch has not been deployed

No deployments
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.

_promote_float casts 32/64-bit integer rasters to float32, losing integer precision above 2**24

1 participant