Explore/fft backends - #373
Merged
Merged
Conversation
….fft Explores whether pyFFTW (FFTW bindings), rocket-fft (numba-jitted numpy.fft), or a hand-rolled Cython FFT wrapper could beat the scipy.fft backend merged in v0.25.5. Result: none of them win by enough to justify a new dependency. - pyFFTW (default ESTIMATE planning): a wash vs scipy.fft (0.88-1.05x). - pyFFTW (FFTW_MEASURE, plan reused across repeated calls of the same shape): ~7% faster in steady state, but the ~1s one-time planning cost needs 250+ calls at a fixed shape to amortize -- not representative of a multipass PIV run using several window sizes per pass. - rocket-fft (numba): consistently 2x slower than scipy.fft. - Custom Cython FFT wrapper: not built. It would at best match pyFFTW (same underlying FFTW library and planning cost), which already falls short of scipy.fft's real-world advantage -- ruled out by the benchmark numbers before writing any Cython. See benchmarks/fft_backends/RESULTS.md for full numbers and methodology. Recommendation: keep scipy.fft as shipped in v0.25.5; do not add pyfftw/ numba/rocket-fft as dependencies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEo1qh1WnHYS4eAkq449yu
…arking suite - Implement Rust backend (openpiv_rust) with Rayon-parallelized RealFFT circular and linear cross-correlation and subpixel peak position finding - Fix linear cross-correlation power-of-2 centering formula and workers parameter for SciPy backend - Integrate backend='rust' into pyprocess, multigrid_windef, and multiprocessing tools - Add GitHub Actions workflows for multi-platform binary wheel builds (wheels.yml) and dual-backend testing (testing.yml) - Add developer guide in documentation and benchmark suite (benchmarks/run_benchmarks.py)
…IV packages to PyPI - Consolidate packaging into .github/workflows/wheels.yml - Build openpiv_rust wheels for Linux (x86_64, aarch64), Windows (x64), and macOS (x86_64, aarch64) via PyO3/maturin-action - Build openpiv pure Python sdist and wheel via Poetry - Publish all distributions to PyPI using pypa/gh-action-pypi-publish on release tags or workflow_dispatch
- Use astral-sh/setup-uv@v5 across all GitHub Actions workflows with caching - Replace poetry build with uv build in wheels.yml - Replace poetry publish with uv publish in wheels.yml - Replace poetry install with uv python install and uv pip in testing.yml - Modernize tool.poetry.group.dev.dependencies in pyproject.toml
Reviewer's GuideThis PR introduces an optional Rust native backend for batched FFT correlation and subpixel displacement processing, wires backend selection through the PIV pipeline, adds multiprocessing and in-memory windef support, and supplies validation, benchmarks, documentation, and CI/wheel publishing for the new implementation. Sequence diagram for backend-selected PIV correlationsequenceDiagram
participant PIV as PIV pipeline
participant PyProcess as pyprocess.fft_correlate_images
participant SciPy as scipy.fft
participant Rust as openpiv_rust
PIV->>PyProcess: fft_correlate_images(..., backend)
alt backend is rust
PyProcess->>Rust: fft_correlate_circular or fft_correlate_linear
Rust->>Rust: batched FFT correlation
Rust-->>PyProcess: correlation maps
else backend is scipy
PyProcess->>SciPy: rfft2 and irfft2
SciPy-->>PyProcess: correlation maps
end
PyProcess-->>PIV: correlation maps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 4 other issues
Security issues:
- PyO3 has an Out-of-bounds Read in
nth/nth_backforPyListandPyTupleiterators (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/openpiv_rust/src/lib.rs" line_range="187-189" />
<code_context>
+ // 5. Normalization scale factor + 2D fftshift
+ let fft_norm = 1.0 / ((h * w) as f64);
+ let scale = if normalized_correlation {
+ fft_norm * fft_norm
+ } else {
+ fft_norm
+ };
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The Rust backend applies an extra normalization factor for normalized correlations: circular mode uses `1/(H*W)^2` after the inputs have already been normalized, and linear mode divides the result by the output dimensions after the inverse FFT. Rust normalized results are therefore smaller than SciPy results by an additional image-size-dependent factor.
**Triggers:** When `backend='rust'` and `normalized_correlation=True`.
**Suggested fix:** Use the same inverse-FFT scaling as SciPy after `normalize_intensity`; remove the extra `fft_norm` factor in circular mode and the post-correlation division in linear mode.
</issue_to_address>
### Comment 2
<location path="openpiv/pyprocess.py" line_range="435-439" />
<code_context>
if subpixel_method not in ("gaussian", "centroid", "parabolic"):
raise ValueError(f"Method not implemented {subpixel_method}")
+ if HAS_RUST and corr.ndim == 2:
+ return openpiv_rust.find_subpixel_peak_position(
+ np.ascontiguousarray(corr, dtype=np.float64),
+ subpixel_method=subpixel_method,
+ )
+
# the peak locations
</code_context>
<issue_to_address>
**issue (testing):** The Rust subpixel test does not compare Rust against the Python implementation because `pyprocess.find_subpixel_peak_position` dispatches to Rust whenever `openpiv_rust` is installed. Both sides of the assertion therefore execute the same Rust code, allowing a regression in the Rust implementation to pass unnoticed.
**Triggers:** When the Rust extension is installed, as required by the Rust test environment.
**Suggested fix:** Call the pre-dispatch Python implementation directly or disable the automatic Rust dispatch while computing the reference result.
</issue_to_address>
### Comment 3
<location path="openpiv/pyprocess.py" line_range="435-439" />
<code_context>
if subpixel_method not in ("gaussian", "centroid", "parabolic"):
raise ValueError(f"Method not implemented {subpixel_method}")
+ if HAS_RUST and corr.ndim == 2:
+ return openpiv_rust.find_subpixel_peak_position(
+ np.ascontiguousarray(corr, dtype=np.float64),
+ subpixel_method=subpixel_method,
+ )
+
# the peak locations
</code_context>
<issue_to_address>
**issue (broader_impact):** Installing the optional Rust package silently changes the default SciPy/Python execution path for subpixel peak finding and displacement conversion, even when the caller selects the SciPy backend. The Rust implementation is selected solely from `HAS_RUST`, so users can get different numerical behavior merely by installing the optional extension.
**Triggers:** When `openpiv_rust` is installed and callers use the default SciPy backend.
**Suggested fix:** Gate Rust subpixel dispatch on an explicit backend setting and propagate that setting through `correlation_to_displacement` and related callers.
</issue_to_address>
### Comment 4
<location path="benchmarks/fft_backends/bench_single_call.py" line_range="4" />
<code_context>
+"""Benchmark FFT backends for openpiv's batched cross-correlation pattern.
+
+Not part of the package; a throwaway script for the explore/fft-backends
+branch. Run with: uv run python scripts_bench_fft.py
+"""
+import time
</code_context>
<issue_to_address>
**nitpick:** The benchmark module's usage documentation tells users to run `scripts_bench_fft.py`, but that file is not the added benchmark script. Following the documented command fails because the referenced entry point does not exist.
**Suggested fix:** Change the usage example to `python benchmarks/fft_backends/bench_single_call.py` or provide the referenced wrapper script.
```suggestion
branch. Run with: python benchmarks/fft_backends/bench_single_call.py
```
</issue_to_address>
### Comment 5
<location path="crates/openpiv_rust/Cargo.lock" line_range="196-212" />
<code_context>
</code_context>
<issue_to_address>
**security (GHSA-36hh-v3qg-5jq4):** PyO3 has an Out-of-bounds Read in `nth` / `nth_back` for `PyList` and `PyTuple` iterators
PyO3 0.24.0 added optimized implementations of `Iterator::nth` and `DoubleEndedIterator::nth_back` for the `BoundListIterator` and `BoundTupleIterator` types. These implementations computed the target index using unchecked `usize` addition (`index + n`) before bounds-checking against the sequence length, then read the element via `get_item_unchecked`.
In `nth` methods, a sufficiently large `n` (combined with a non-zero internal index) could cause the addition to overflow and wrap around, producing a small "target index" that passed the bounds check and enabling reads at the front of the `list` or `tuple` of elements previously yielded by the iterator.
In `nth_back` methods, a sufficiently large `n` could cause underflow in a similar fashion, however would instead allow reads of arbitrary memory past the end of the `list` or `tuple` storage.
*Source: trivy*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and the new workflow grants write and OIDC token permissions and can publish artifacts from a manually selected branch to PyPI, so a mistaken trigger or compromised dependency could create an externally distributed release that reverting the PR cannot retract. The Rust backend and CI changes also add substantial new build and dependency surface beyond the existing SciPy path.
Blocking findings: crates/openpiv_rust/src/lib.rs:189, openpiv/pyprocess.py:439, openpiv/pyprocess.py:439, crates/openpiv_rust/Cargo.lock:212
Comment on lines
+187
to
+189
| fft_norm * fft_norm | ||
| } else { | ||
| fft_norm |
There was a problem hiding this comment.
issue (bug_risk): The Rust backend applies an extra normalization factor for normalized correlations: circular mode uses 1/(H*W)^2 after the inputs have already been normalized, and linear mode divides the result by the output dimensions after the inverse FFT. Rust normalized results are therefore smaller than SciPy results by an additional image-size-dependent factor.
Triggers: When backend='rust' and normalized_correlation=True.
Suggested fix: Use the same inverse-FFT scaling as SciPy after normalize_intensity; remove the extra fft_norm factor in circular mode and the post-correlation division in linear mode.
Comment on lines
+435
to
+439
| if HAS_RUST and corr.ndim == 2: | ||
| return openpiv_rust.find_subpixel_peak_position( | ||
| np.ascontiguousarray(corr, dtype=np.float64), | ||
| subpixel_method=subpixel_method, | ||
| ) |
There was a problem hiding this comment.
issue (testing): The Rust subpixel test does not compare Rust against the Python implementation because pyprocess.find_subpixel_peak_position dispatches to Rust whenever openpiv_rust is installed. Both sides of the assertion therefore execute the same Rust code, allowing a regression in the Rust implementation to pass unnoticed.
Triggers: When the Rust extension is installed, as required by the Rust test environment.
Suggested fix: Call the pre-dispatch Python implementation directly or disable the automatic Rust dispatch while computing the reference result.
Comment on lines
+435
to
+439
| if HAS_RUST and corr.ndim == 2: | ||
| return openpiv_rust.find_subpixel_peak_position( | ||
| np.ascontiguousarray(corr, dtype=np.float64), | ||
| subpixel_method=subpixel_method, | ||
| ) |
There was a problem hiding this comment.
issue (broader_impact): Installing the optional Rust package silently changes the default SciPy/Python execution path for subpixel peak finding and displacement conversion, even when the caller selects the SciPy backend. The Rust implementation is selected solely from HAS_RUST, so users can get different numerical behavior merely by installing the optional extension.
Triggers: When openpiv_rust is installed and callers use the default SciPy backend.
Suggested fix: Gate Rust subpixel dispatch on an explicit backend setting and propagate that setting through correlation_to_displacement and related callers.
| """Benchmark FFT backends for openpiv's batched cross-correlation pattern. | ||
|
|
||
| Not part of the package; a throwaway script for the explore/fft-backends | ||
| branch. Run with: uv run python scripts_bench_fft.py |
There was a problem hiding this comment.
nitpick: The benchmark module's usage documentation tells users to run scripts_bench_fft.py, but that file is not the added benchmark script. Following the documented command fails because the referenced entry point does not exist.
Suggested fix: Change the usage example to python benchmarks/fft_backends/bench_single_call.py or provide the referenced wrapper script.
Suggested change
| branch. Run with: uv run python scripts_bench_fft.py | |
| branch. Run with: python benchmarks/fft_backends/bench_single_call.py |
- Set PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 and pin supported Python interpreters in maturin wheel builds - Use pytest.importorskip for openpiv_rust in unit tests to allow pure Python CI without Rust - Fix uv virtualenv usage and pass --system in copilot setup steps - Use explicit pytest executable path from uv venv in testing.yml
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
🟡 Changes recommended
CI and local SciPy-only test runs will fail due to unconditional Rust-test imports and an invalid .[dev] install target in the uv-based workflow.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an optional Rust-accelerated FFT correlation backend (openpiv_rust) while keeping SciPy as the default, and wires backend selection + multiprocessing controls through PIVSettings and the windef pipeline. It also expands CI/build workflows (uv + maturin) and adds documentation/benchmarks for backend behavior and performance.
Changes:
- Add Rust-backed FFT correlation + batched subpixel/displacement helpers, selectable via
settings.backend, and propagate backend selection throughwindef/pyprocess. - Refactor windef batch processing for multiprocessing picklability and improve multiprocessing cleanup with a context-managed pool.
- Add Rust backend tests, docs/benchmarks, and new wheel-building/publishing workflows.
File summaries
| File | Description |
|---|---|
uv.lock |
Updates uv lock metadata (Python requirement). |
pyproject.toml |
Adds optional openpiv-rust dependency, rust extra, and dev-group deps (incl. maturin). |
openpiv/windef.py |
Refactors batch worker, adds backend passthrough to correlation, adds multigrid_windef. |
openpiv/tools.py |
Uses context-managed multiprocessing.Pool to ensure cleanup. |
openpiv/test/test_rust_windef.py |
Adds windef Rust-vs-SciPy parity + multiprocessing tests. |
openpiv/test/test_rust_subpixel.py |
Adds Rust subpixel + batch displacement tests. |
openpiv/test/test_rust_backend.py |
Adds Rust FFT correlation correctness/edge-case tests. |
openpiv/settings.py |
Adds backend and n_cpus settings for backend selection and multiprocessing. |
openpiv/pyprocess.py |
Adds Rust acceleration hooks, backend selection for FFT correlation, and workers plumbing for SciPy FFT. |
openpiv/docs/src/fft_correlation_backends.rst |
New backend/performance study documentation. |
openpiv/docs/src/developers.rst |
Adds Rust vs non-Rust development instructions. |
openpiv/docs/index.rst |
Adds the new FFT backend doc to the Sphinx toctree. |
crates/openpiv_rust/src/lib.rs |
Implements Rust FFT correlation + subpixel/displacement APIs via PyO3/Rayon/RealFFT. |
crates/openpiv_rust/Cargo.toml |
Declares Rust crate metadata and dependencies. |
crates/openpiv_rust/Cargo.lock |
Locks Rust crate dependency versions. |
benchmarks/run_benchmarks.py |
Adds benchmark runner for correlation/subpixel/windef backends. |
benchmarks/fft_backends/test_rust_fft.py |
Adds standalone Rust vs SciPy validation/benchmark script. |
benchmarks/fft_backends/run_piv_quiver_demo.py |
Adds demo script for visual comparison of vector fields. |
benchmarks/fft_backends/RESULTS.md |
Records backend exploration findings/results. |
benchmarks/fft_backends/benchmark_windef.py |
Adds windef-focused benchmark harness. |
benchmarks/fft_backends/bench_single_call.py |
Adds single-call FFT backend benchmark script. |
benchmarks/fft_backends/bench_repeated_calls.py |
Adds repeated-call FFT backend benchmark script. |
.gitignore |
Ignores Rust build outputs and wheel artifacts. |
.github/workflows/wheels.yml |
New multi-OS wheel build + optional PyPI publishing workflow. |
.github/workflows/testing.yml |
Migrates CI testing to uv and adds a Rust-backend job. |
.github/workflows/copilot-setup-steps.yml |
Updates setup steps to uv and newer checkout action. |
.github/workflows/build.yml |
Removes the previous Poetry-based publish workflow. |
Review details
Suppressed comments (1)
.github/workflows/copilot-setup-steps.yml:1
- This workflow file begins with a UTF-8 BOM before
name:. Remove the BOM to prevent YAML parsing/tooling issues across environments.
name: "Copilot Setup Steps"
- Files reviewed: 24/27 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - name: Install dependencies (pure Python / SciPy mode) | ||
| run: | | ||
| uv venv --python ${{ matrix.python-version }} | ||
| uv pip install ".[dev]" pytest |
Comment on lines
+1
to
+4
| import numpy as np | ||
| import pytest | ||
| import openpiv_rust | ||
| from openpiv import pyprocess |
Comment on lines
+4
to
+6
| import numpy as np | ||
| import pytest | ||
| from imageio.v3 import imwrite |
…& CI check, wire Rust acceleration for median & windowing
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.
Summary by Sourcery
Add an optional Rust acceleration backend for OpenPIV processing while retaining SciPy fallback behavior and supporting cross-platform packaging, benchmarking, documentation, and CI validation.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Deployment:
Documentation: