Skip to content

vectorized_sig2noise_ratio never applies its validity flag (flag is True indexing is a no-op) #368

@fbanelli

Description

@fbanelli

Bug

In pyprocess.vectorized_sig2noise_ratio, the validity flag (weak first peak, border first peak, and for peak2peak a weak/border second peak) is carefully constructed but then never applied:

peak2peak[flag is True] = 0 # replace invalid values
...
peak2mean[flag is True] = 0 # replace invalid values

flag is True is a Python identity comparison between an ndarray and True, which evaluates to the constant False. Numpy treats a scalar boolean index as a mask of shape (), so arr[False] selects an empty view and the assignment is a silent no-op:

>>> import numpy as np
>>> a = np.arange(1.0, 5.0)
>>> flag = np.array([True, False, True, False])
>>> a[flag is True] = 0
>>> a
array([1., 2., 3., 4.])   # unchanged
>>> a[flag is True].shape
(0, 4)

Impact

Invalid windows (no signal, or peaks on the correlation-map border) leak their raw peaks1/peaks2 (or peaks1/mean) ratios into the returned signal-to-noise array instead of the intended 0. Anything thresholding on s2n downstream (e.g. validation.sig2noise_val) can therefore keep windows the function meant to reject. The loop-based sig2noise_ratio does zero out failed windows, so the two implementations disagree on exactly the windows the flag is for.

Repro on the function itself

import numpy as np
from openpiv.pyprocess import vectorized_sig2noise_ratio

corr = np.full((1, 16, 16), 0.05)
corr[0, 0, 5] = 1.0   # first peak ON the border -> flagged
corr[0, 8, 8] = 0.5

print(vectorized_sig2noise_ratio(corr, 'peak2peak', width=1))  # expected [0.], got [2.] on 0.25.4/master

Fix

Two-line change: peak2peak[flag] = 0 and peak2mean[flag] = 0. PR incoming.

One heads-up for review: test_vectorized_sig2noise_ratio currently passes 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 every peak2peak ratio in that test becomes 0. The PR therefore also rebuilds that test on 16×16 maps with interior peaks and adds an explicit regression test for the flag behaviour.

Also worth noting (left out of the PR on purpose): the vectorized flag is stricter than the loop-based sig2noise_ratio, which only rejects a border second peak when corr_max2 > 0.5 * corr_max1. Aligning the two semantics is a separate design decision for the maintainers.

Found while porting the function to JAX and pinning parity tests against the reference.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions