Skip to content

Promote wide integer rasters to float64 in edge_detection (#3680)#3686

Merged
brendancol merged 4 commits into
mainfrom
deep-sweep-security-edge_detection-2026-07-18-01
Jul 18, 2026
Merged

Promote wide integer rasters to float64 in edge_detection (#3680)#3686
brendancol merged 4 commits into
mainfrom
deep-sweep-security-edge_detection-2026-07-18-01

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3680

convolve_2d casts 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.

Backend coverage: numpy, cupy, dask+numpy, dask+cupy (all four executed locally, CUDA host).

Test plan:

  • New 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.
  • Full test_edge_detection.py passes: 117 tests (87 existing + 30 new).
  • flake8 on touched files: no new violations (the 4 E201s are pre-existing, tracked in Style: flake8 E201 in edge_detection kernel literals #3675).

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 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 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.py docstrings: "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_int helper docstring states the 253 bound; the five public docstrings could carry the same qualifier since that's what users read.

Nits (optional improvements)

  • _to_numpy in the test file re-implements the backend extraction that general_output_checks does with an ArrayTypeFunctionMapping. Fine as a local helper, but if a third test file grows one of these it belongs in general_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.
  • astype on the DataArray's .data stays 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_nonzero pins the exact silent-failure mode from #3680 (all-zero interior), not just closeness to a reference.
  • Scoping the shared _promote_float question 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 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: 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_numpy duplicates general_checks extraction): dismissed for this PR. It is a four-line test-local helper; promoting it to general_checks.py is only worth doing when a second consumer appears.

No new findings in the follow-up diff. Nothing blocking.

@brendancol
brendancol merged commit 60b2f1a into main Jul 18, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

edge_detection: integer rasters with values above 2**24 give silently zeroed gradients

1 participant