Repository navigation
Promote 32/64-bit integer rasters to float64 in _promote_float (#3684) - #3764
Open
brendancol wants to merge 2 commits into
Open
brendancol wants to merge 2 commits into
brendancol wants to merge 2 commits into
Conversation
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.
brendancol
commented
Nov 6, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
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_intdocstring still says "convolve_2dpromotes 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 overbackendlike 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, wherecreate_test_rasterignores it. Harmless, just noise. -
docs/source/user_guide/data_types.rst:12still 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
applyandfocal_statscomputeitemsizefrom_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, notallclose. 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.
hotspotsis 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
commented
Nov 6, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: follow-up after d124db6
All four findings from the first pass are addressed in d124db6:
edge_detection._promote_wide_intdocstring no longer claimsconvolve_2dpromotes to float32; it says the pre-cast is redundant since #3684 and why it stays.test_convolution_2d_wide_int_exact_above_2_24_3684is 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.rstopening 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
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 #3684
_promote_floatinxrspatial/convolution.pymapped 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, andconvolve_2d,convolution_2d,focal.mean,applyandfocal_statsgave 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_statsalready size themselves from_promote_float, so they pick that up automatically.Docs touched: the
meanandconvolve_2ddocstrings, the focal memory-budget comments, the note infocal.rst, and the data-types guide. Theconvolution_2ddocstring examples lose theirdtype=float32tag becausenp.arangeinput is int64 and now comes out float64.Tests:
test_mean_int_promotes_to_float32_3214becomes a parametrized promotion test over narrow and wide ints, and the three #3217 dtype tests compute their expectation through_promote_floatinstead 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_intfrom #3680 pre-casts wide ints to float64 before callingconvolve_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)