Skip to content

Add asv benchmarks for edge_detection (#3672)#3673

Open
brendancol wants to merge 2 commits into
mainfrom
deep-sweep-benchmarks-edge_detection-2026-07-18
Open

Add asv benchmarks for edge_detection (#3672)#3673
brendancol wants to merge 2 commits into
mainfrom
deep-sweep-benchmarks-edge_detection-2026-07-18

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3672.

  • Adds benchmarks/benchmarks/edge_detection.py with an EdgeDetection class covering all five public functions (sobel_x, sobel_y, prewitt_x, prewitt_y, laplacian), parameterized over nx in [300, 3000] and numpy/cupy/dask via the suite's get_xr_dataarray.
  • Benchmark-only change, no source edits. Also carries the benchmark sweep's state CSV row.

Backend coverage: numpy, cupy, dask+numpy. dask+cupy is not parameterizable with the current get_xr_dataarray (common.py has no such type), same as the rest of the suite.

Test plan:

  • All 30 function/size/backend combinations executed locally by direct class invocation, including cupy on a GPU host. Timings range from sub-ms (cupy) to ~60 ms (dask at nx=3000), so added CI runtime is small.
  • flake8 and isort clean on the new file.

@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: Add asv benchmarks for edge_detection (#3672)

Blockers (must fix before merge)

  • None.

Suggestions (should fix, not blocking)

  • None.

Nits (optional improvements)

  • benchmarks/benchmarks/edge_detection.py:14-19: the cupy timings don't synchronize the device after the call, so for async numba kernel launches time_* can measure launch overhead rather than kernel time. Every other benchmark in the suite (common.Benchmarking, Convolve2d) does the same, so fixing it only here would make the numbers incomparable. Fine to leave; a suite-wide sync pass would be its own change.

What looks good

  • setup goes through get_xr_dataarray, so a missing cupy raises NotImplementedError and asv records a skip instead of an error.
  • The dask result is forced with .compute() (checked via hasattr on result.data), so the benchmark times the kernel, not graph construction. numpy and cupy results pass through untouched.
  • Sizes ([300, 3000]) follow the "one small, one realistic" convention and keep added CI runtime low; the PR body reports all 30 combos executed locally, including cupy on a GPU host.
  • The state CSV row is one physical line and matches the sweep schema.

Checklist

  • Algorithm matches reference/paper (n/a, benchmark-only)
  • All implemented backends produce consistent results (n/a, no source change)
  • NaN handling is correct (n/a)
  • Edge cases are covered by tests (n/a, benchmark-only)
  • Dask chunk boundaries handled correctly (default get_xr_dataarray 2x2 chunking, real chunked execution)
  • No premature materialization (.compute() only where the timing requires it)
  • Benchmark exists or is not needed (this PR is the benchmark)
  • README feature matrix updated (n/a, no new functions)
  • Docstrings present and accurate (n/a, benchmark classes follow suite style)

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 has no asv benchmarks

1 participant