fix: apply validity flag in vectorized_sig2noise_ratio#369
Open
fbanelli wants to merge 1 commit into
Open
Conversation
'peak2peak[flag is True] = 0' indexes with the Python expression 'flag is True' (a constant False), which numpy treats as an empty boolean mask, so the assignment was a silent no-op and invalid windows (no signal, border peaks) leaked raw ratios instead of the intended 0. Same for peak2mean. Index with the flag array itself. The existing test only passed because of the no-op: on its 5x5 correlation maps the second peak always sits on the map border, so applying the flag zeroes every peak2peak ratio. Rebuild it on 16x16 maps with interior peaks and add an explicit regression test for the flag behaviour. Fixes OpenPIV#368 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideFixes the application of the validity flag in vectorized_sig2noise_ratio so invalid correlation windows yield zero signal-to-noise ratios as intended, and expands/adjusts tests to cover and verify both valid and invalid window behavior for peak2peak and peak2mean methods. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new regression test
test_vectorized_sig2noise_ratio_flags_invalid_windowsbakes in the internal1e-3threshold for a valid peak; consider referencing the threshold symbolically (or centralizing it) so the test doesn’t become brittle if that value is adjusted in the implementation. - Since the vectorized implementation intentionally has stricter semantics than the loop-based
sig2noise_ratio(e.g., border second peak handling), it may be helpful to add a brief comment invectorized_sig2noise_ratioitself describing this difference so future maintainers don’t assume they are identical.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new regression test `test_vectorized_sig2noise_ratio_flags_invalid_windows` bakes in the internal `1e-3` threshold for a valid peak; consider referencing the threshold symbolically (or centralizing it) so the test doesn’t become brittle if that value is adjusted in the implementation.
- Since the vectorized implementation intentionally has stricter semantics than the loop-based `sig2noise_ratio` (e.g., border second peak handling), it may be helpful to add a brief comment in `vectorized_sig2noise_ratio` itself describing this difference so future maintainers don’t assume they are identical.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Fixes #368.
What
vectorized_sig2noise_ratiobuilds a per-window validity flag (weak first peak, border first peak, and forpeak2peaka weak/border second peak) but never applies it:This PR indexes with the flag array itself (
peak2peak[flag] = 0,peak2mean[flag] = 0), so invalid windows return 0 as intended and as the loop-basedsig2noise_ratioalready does.Test changes
test_vectorized_sig2noise_ratiopassed only because of the no-op: its 5×5 correlation maps are so small that the second peak always lands on the map border, so with the flag applied everypeak2peakratio in that test becomes 0 and thes2n[0] > s2n[2]assertion fails. Rebuilt on 16×16 maps with interior peaks (same assertions, plus> 0checks that no window is flagged).test_vectorized_sig2noise_ratio_flags_invalid_windowscovering: healthy window stays positive, border first peak → 0, no-signal window → 0, border second peak → 0 forpeak2peak(and correctly not zeroed forpeak2mean).Behavior change note
Downstream code thresholding on s2n (e.g.
validation.sig2noise_val) will now correctly reject windows the function always meant to reject; previously those leaked raw ratios. Note the vectorized flag is stricter than the loop-basedsig2noise_ratio(which rejects a border second peak only whencorr_max2 > 0.5 * corr_max1); aligning those two semantics is left to a separate discussion, as noted in #368.Verification
test_pyprocess.py(12),test_process.py+test_validation.py+test_lib.py(37) all pass locally against the patched source (numpy 1.x, CPU).🤖 Generated with Claude Code
Summary by Sourcery
Ensure vectorized signal-to-noise ratio computation correctly zeroes out invalid windows and strengthen its regression test coverage.
Bug Fixes:
Tests: