Promote wide integer rasters to float64 in edge_detection (#3680)#3686
Merged
brendancol merged 4 commits intoJul 18, 2026
Conversation
convolve_2d casts every integer dtype to float32, whose 24-bit mantissa cannot separate integers above 2**24, so sobel/prewitt/laplacian on int32/int64 rasters with large values returned all-zero gradients with no warning. Pre-promote 32/64-bit integers to float64 before the convolution; 8/16-bit integers are exact in float32 and keep the existing path.
brendancol
commented
Jul 18, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Promote wide integer rasters to float64 in edge_detection (#3680)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_edge_detection.py(test_output_dtype_by_input_dtype): the dtype table covers int8/int16/uint16/int32/uint32/int64 plus the two floats but skips uint8 and uint64. Both hit the same branches, so it's two cheap rows that make the table actually exhaustive over the kinds the helper switches on. -
xrspatial/edge_detection.pydocstrings: "32/64-bit integers as float64 so that large values keep unit precision" overpromises slightly for int64/uint64, where float64 is only exact up to 253. The_promote_wide_inthelper docstring states the 253 bound; the five public docstrings could carry the same qualifier since that's what users read.
Nits (optional improvements)
-
_to_numpyin the test file re-implements the backend extraction thatgeneral_output_checksdoes with anArrayTypeFunctionMapping. Fine as a local helper, but if a third test file grows one of these it belongs ingeneral_checks.py.
What looks good
- The fix targets the actual defect: pre-promoting only 32/64-bit ints keeps 8/16-bit inputs on the established float32 contract (#1096/#2769/#3214) instead of quietly widening everything.
astypeon the DataArray's.datastays lazy on dask and works unchanged on cupy, so all four backends are covered by one code path; the parametrized regression test executes all four.test_large_int32_gradient_nonzeropins the exact silent-failure mode from #3680 (all-zero interior), not just closeness to a reference.- Scoping the shared
_promote_floatquestion to #3684 rather than reaching into convolution/focal from this PR is the right call given the contract test in test_focal.py.
Checklist
- Algorithm matches reference/paper (no kernel math changed; promotion arithmetic checked against float32/float64 mantissa widths)
- All implemented backends produce consistent results (parametrized test, all four executed on this host)
- NaN handling is correct (untouched; existing NaN tests still pass)
- Edge cases are covered by tests (int32 above 2**24, int64 at 1e10, 8-dtype output table)
- Dask chunk boundaries handled correctly (no depth/boundary changes)
- No premature materialization or unnecessary copies (lazy astype; one cast only for wide-int inputs)
- Benchmark exists or is not needed — none exists for edge_detection at all; tracked separately in #3672, not this PR's job
- README feature matrix updated (n/a: no new function, no backend change)
- Docstrings present and accurate (modulo the 2**53 qualifier above)
brendancol
commented
Jul 18, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review follow-up: Promote wide integer rasters to float64 in edge_detection (#3680)
Disposition of the first-pass findings, re-checked against 033fda8:
- Suggestion (dtype table missing uint8/uint64): fixed. The table now has all eight integer dtypes plus float32/float64; 119 tests pass.
- Suggestion (docstrings overpromise for 64-bit ints): fixed. All five public docstrings now carry the "exact up to 2**53 for 64-bit integers" qualifier, matching the helper docstring.
- Nit (
_to_numpyduplicates general_checks extraction): dismissed for this PR. It is a four-line test-local helper; promoting it togeneral_checks.pyis only worth doing when a second consumer appears.
No new findings in the follow-up diff. Nothing blocking.
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 #3680
convolve_2dcasts every integer dtype to float32, whose 24-bit mantissa cannot separate integers above 2**24. Sobel, Prewitt, and Laplacian on int32/int64 rasters with large values (offset 1e8 in the repro) returned all-zero gradients on every backend, with no warning._promote_wide_intinedge_detection.py: 32/64-bit integer inputs are cast to float64 beforeconvolve_2d. 8/16-bit integers are exact in float32 and keep the existing path.astypeis lazy on dask, so no materialization._promote_floatcontract shared with convolution/focal is _promote_float casts 32/64-bit integer rasters to float32, losing integer precision above 2**24 #3684.Backend coverage: numpy, cupy, dask+numpy, dask+cupy (all four executed locally, CUDA host).
Test plan:
TestWideIntegerPrecision: int32 ramp at offset 1e8 matches the float64 reference on all four backends; interior gradients nonzero (the exact silent failure); int64 at offset 1e10; output-dtype table for 8 input dtypes.test_edge_detection.pypasses: 117 tests (87 existing + 30 new).