Skip to content

Honor the boundary parameter on the cupy backend in bilateral()#3632

Open
brendancol wants to merge 3 commits into
mainfrom
issue-3625
Open

Honor the boundary parameter on the cupy backend in bilateral()#3632
brendancol wants to merge 3 commits into
mainfrom
issue-3625

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3625

  • The cupy backend dropped boundary: in the _bilateral dispatcher, cupy_func was the only entry not wrapped with partial(..., 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.
  • Adds _bilateral_cupy_boundary, mirroring _bilateral_numpy_boundary (_pad_array already handles cupy arrays via cupy.pad), and wires it through the dispatcher.
  • Adds test_bilateral_boundary_modes_gpu, which compares cupy against numpy for all four modes. The existing assert_boundary_mode_correctness helper only covers numpy vs dask+numpy, which is why this went unnoticed.
  • Updates the accuracy-sweep state CSV for the bilateral module.

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.py on a CUDA host: 23 passed (cupy and dask+cupy paths execute, not skipped)
  • New GPU boundary test fails without the fix (max diff 2.9 for 'nearest') and passes with it

@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: 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 default boundary='nan' (test_bilateral_dask_gpu). The dask+cupy path forwards boundary through _boundary_to_dask, so it works today, but nothing pins it. Extending the new test (or the existing dask+cupy test) to loop over VALID_BOUNDARY_MODES would 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 of assert_boundary_mode_correctness, so this is a style call.

What looks good

  • _bilateral_cupy_boundary (xrspatial/bilateral.py:188) mirrors _bilateral_numpy_boundary exactly: same pad-filter-slice structure, same 'nan' short-circuit. _pad_array already dispatches to cupy.pad for cupy inputs, so no new GPU code was needed.
  • The result[radius:-radius, radius:-radius] slice is safe: radius >= 1 is guaranteed because sigma_spatial is validated against sqrt(finfo.tiny) and ceil(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_func now gets the same partial(..., 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 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 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_gpu compares dask+cupy against numpy for every mode in VALID_BOUNDARY_MODES, chunked (15, 15) so chunk seams cross the kernel footprint. rtol 1e-4 matches the existing test_bilateral_dask_gpu tolerance.
  • 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.

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.

bilateral() ignores the boundary parameter on the cupy backend

1 participant