Honor the boundary parameter on the cupy backend in bilateral()#3632
Open
brendancol wants to merge 3 commits into
Open
Honor the boundary parameter on the cupy backend in bilateral()#3632brendancol wants to merge 3 commits into
brendancol wants to merge 3 commits into
Conversation
brendancol
commented
Jul 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Honor the boundary parameter on the cupy backend in bilateral()
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/tests/test_bilateral.py:337-- the new parity test covers numpy vs cupy for all four modes, but dask+cupy is still only exercised at the defaultboundary='nan'(test_bilateral_dask_gpu). The dask+cupy path forwardsboundarythrough_boundary_to_dask, so it works today, but nothing pins it. Extending the new test (or the existing dask+cupy test) to loop overVALID_BOUNDARY_MODESwould lock all four backends for a few extra lines.
Nits (optional improvements)
-
xrspatial/tests/test_bilateral.py:345-- the for-loop over modes means the first failing mode hides the rest.@pytest.mark.parametrize('mode', VALID_BOUNDARY_MODES)would report each mode separately. The loop does match the style ofassert_boundary_mode_correctness, so this is a style call.
What looks good
_bilateral_cupy_boundary(xrspatial/bilateral.py:188) mirrors_bilateral_numpy_boundaryexactly: same pad-filter-slice structure, same'nan'short-circuit._pad_arrayalready dispatches tocupy.padfor cupy inputs, so no new GPU code was needed.- The
result[radius:-radius, radius:-radius]slice is safe:radius >= 1is guaranteed becausesigma_spatialis validated againstsqrt(finfo.tiny)andceil(2*sigma)of any accepted value is at least 1, and the extent clamp never drops below 1. - The dispatcher change (xrspatial/bilateral.py:228) is the minimal fix:
cupy_funcnow gets the samepartial(..., boundary=boundary)treatment as the other three entries. - The regression test fails on main (max diff ~2.9 for
'nearest'on data with std 10) and passes here; cupy now matches numpy to ~5e-14 per the PR body. - State CSV diff is a single-row change, no whole-file churn.
Checklist
- Algorithm matches reference/paper (pad-filter-crop equivalent to the numpy wrapper)
- All implemented backends produce consistent results (numpy/cupy verified for all modes on a CUDA host; dask+numpy via existing boundary test; dask+cupy at default mode only, see suggestion)
- NaN handling is correct (unchanged;
'nan'path still short-circuits to the clamped-window kernel) - Edge cases are covered by tests (existing single-cell, all-NaN, oversize-sigma tests still pass)
- Dask chunk boundaries handled correctly (untouched by this PR)
- No premature materialization or unnecessary copies (one pad copy, same as the numpy path)
- Benchmark exists or is not needed (bug fix, no perf-relevant change; no bilateral benchmark exists today)
- README feature matrix updated (not applicable, no backend support change at the matrix level)
- Docstrings present and accurate (public docstring already documents the boundary modes this PR makes true on cupy)
brendancol
commented
Jul 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: follow-up pass (commit 77e2e75)
Re-review after the review follow-ups. Both earlier findings are addressed:
- Suggestion (dask+cupy boundary coverage): fixed. New
test_bilateral_boundary_modes_dask_gpucompares dask+cupy against numpy for every mode inVALID_BOUNDARY_MODES, chunked (15, 15) so chunk seams cross the kernel footprint. rtol 1e-4 matches the existingtest_bilateral_dask_gputolerance. - Nit (parametrize): fixed. Both GPU tests now use
@pytest.mark.parametrize('mode', VALID_BOUNDARY_MODES)with a module-level import, so each mode reports independently.
Decorator stacking is right (@dask_array_available plus @cuda_and_cupy_available on the dask+cupy test, matching test_bilateral_dask_gpu). Suite is 30 passed on a CUDA host.
No new findings.
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 #3625
boundary: in the_bilateraldispatcher,cupy_funcwas the only entry not wrapped withpartial(..., boundary=boundary), so'nearest','reflect', and'wrap'all behaved as'nan'. Edge-ring results diverged from the other three backends by up to ~30% of a standard deviation on random data._bilateral_cupy_boundary, mirroring_bilateral_numpy_boundary(_pad_arrayalready handles cupy arrays viacupy.pad), and wires it through the dispatcher.test_bilateral_boundary_modes_gpu, which compares cupy against numpy for all four modes. The existingassert_boundary_mode_correctnesshelper only covers numpy vs dask+numpy, which is why this went unnoticed.Backend coverage: numpy, dask+numpy, and dask+cupy are unchanged; cupy now matches numpy to ~5e-14 for every boundary mode.
Test plan:
pytest xrspatial/tests/test_bilateral.pyon a CUDA host: 23 passed (cupy and dask+cupy paths execute, not skipped)'nearest') and passes with it