Open
Conversation
Modified normalization to correctly represent the zero-normalized cross correlation
Updated the normalization method to divide by the standard deviation instead of clipping negative values.
Update tolerance for normalized correlation assertion.
Change dtype from float32 to float64 for mean calculation.
Updated assertion for normalized correlation to allow larger errors. Since standard deviation is not converged on small window
Updated comments in normalize_intensity function to clarify the use of float64 for better accuracy and corrected spelling of 'standard deviation'.
Reviewer's GuideAdjusts normalized cross-correlation to use zero-mean, unit-variance intensity normalization (float64) and correct normalization factors, updating associated tests and docstrings accordingly. Sequence diagram for updated normalized cross-correlation in correlate_windowssequenceDiagram
actor User
participant correlate_windows
participant normalize_intensity
participant fft_correlate_windows
User->>correlate_windows: call correlate_windows(window_a, window_b, correlation_method)
correlate_windows->>normalize_intensity: normalize_intensity(window_a)
normalize_intensity-->>correlate_windows: window_a_norm (zero mean, unit variance)
correlate_windows->>normalize_intensity: normalize_intensity(window_b)
normalize_intensity-->>correlate_windows: window_b_norm (zero mean, unit variance)
correlate_windows->>fft_correlate_windows: compute correlation(window_a_norm, window_b_norm, correlation_method)
fft_correlate_windows-->>correlate_windows: corr_raw
correlate_windows->>correlate_windows: corr = corr_raw/(corr.shape[-2]*corr.shape[-1])
correlate_windows-->>User: corr
Sequence diagram for updated normalized cross-correlation in fft_correlate_imagessequenceDiagram
actor User
participant fft_correlate_images
participant normalize_intensity
User->>fft_correlate_images: call fft_correlate_images(image_a, image_b, correlation_method, normalized_correlation)
fft_correlate_images->>normalize_intensity: normalize_intensity(image_a)
normalize_intensity-->>fft_correlate_images: image_a_norm (zero mean, unit variance)
fft_correlate_images->>normalize_intensity: normalize_intensity(image_b)
normalize_intensity-->>fft_correlate_images: image_b_norm (zero mean, unit variance)
fft_correlate_images->>fft_correlate_images: compute corr_raw via selected correlation_method
alt normalized_correlation is True
fft_correlate_images->>fft_correlate_images: corr = corr_raw/(corr.shape[-2]*corr.shape[-1])
else normalized_correlation is False
fft_correlate_images->>fft_correlate_images: corr = corr_raw
end
fft_correlate_images-->>User: corr
File-Level Changes
Possibly linked issues
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 4 issues, and left some high level feedback:
- In
normalize_intensity, you set the mean’sdtypetonp.float64but rely on the default forstd; consider passingdtype=np.float64tostdas well for consistency and to avoid subtle numerical differences across NumPy versions. - The normalized correlation check in
test_fft_correlate_imagesnow allows values up to 1.5; if possible, it would be better to tighten this bound or add a short comment/logic tying the threshold to window size or expected variance so it doesn’t silently mask genuine regressions. - In
test_normalize_intensity_performance, the assertion message text still refers to(float32)even though the function now returnsfloat64; updating the message would avoid confusion when the test fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `normalize_intensity`, you set the mean’s `dtype` to `np.float64` but rely on the default for `std`; consider passing `dtype=np.float64` to `std` as well for consistency and to avoid subtle numerical differences across NumPy versions.
- The normalized correlation check in `test_fft_correlate_images` now allows values up to 1.5; if possible, it would be better to tighten this bound or add a short comment/logic tying the threshold to window size or expected variance so it doesn’t silently mask genuine regressions.
- In `test_normalize_intensity_performance`, the assertion message text still refers to `(float32)` even though the function now returns `float64`; updating the message would avoid confusion when the test fails.
## Individual Comments
### Comment 1
<location path="openpiv/pyprocess.py" line_range="851" />
<code_context>
print(f"correlation method {correlation_method } is not implemented")
- return corr
+ return corr/(corr.shape[-2]*corr.shape[-1])
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider whether `correlate_windows` should always normalize by window size or offer a flag for raw vs. normalized correlation.
This change always scales by window area, unlike `fft_correlate_images`, where normalization is controlled by `normalized_correlation`. If callers currently rely on raw correlation magnitudes, this becomes a backward-incompatible change and may cause subtle issues (e.g., double-normalization or misread peak heights). Consider adding a `normalized_correlation` parameter here too, or verifying that all call sites expect normalized output.
</issue_to_address>
### Comment 2
<location path="openpiv/pyprocess.py" line_range="755-756" />
<code_context>
"""Normalize interrogation window or strided image of many windows,
- by removing the mean intensity value per window and clipping the
- negative values to zero
+ by removing the mean intensity value per window and dividing by
+ the standard deviation. Note: for small signals the standdeviation
+ might not be full converged. Also numpy docs recommend float64 for
+ better accuracy:
+ https://numpy.org/doc/stable/reference/generated/numpy.std.html
</code_context>
<issue_to_address>
**nitpick (typo):** Fix typos and phrasing in the normalize_intensity docstring for clarity.
Use "standard deviation" and "might not be fully converged" here to improve clarity of the docstring.
```suggestion
def normalize_intensity(window):
"""Normalize an interrogation window or a strided image of many windows
by removing the mean intensity per window and dividing by the
standard deviation.
Note: for small signals the standard deviation might not be fully
converged. NumPy also recommends using float64 for improved
numerical accuracy:
https://numpy.org/doc/stable/reference/generated/numpy.std.html
```
</issue_to_address>
### Comment 3
<location path="openpiv/test/test_performance.py" line_range="34-43" />
<code_context>
def test_normalize_intensity_performance():
"""Test that normalize_intensity avoids unnecessary conversions."""
- # Test with float32 input (should not convert)
- window_float = np.random.rand(50, 64, 64).astype(np.float32)
+ # Test with float64 input (should not convert)
+ window_float = np.random.rand(50, 64, 64).astype(np.float64)
start = time.time()
result = pyprocess.normalize_intensity(window_float)
elapsed_float = time.time() - start
- assert result.dtype == np.float32
+ assert result.dtype == np.float64
# Test with uint8 input (needs conversion)
</code_context>
<issue_to_address>
**suggestion (testing):** Add functional tests for normalize_intensity statistical properties and zero-variance windows
This test now only covers dtype and timing, but the function’s new guarantees (zero mean, unit variance, and zero-std handling) aren’t verified. Please add a separate, non-performance test (e.g. in test_pyprocess.py or a dedicated test_normalize_intensity.py) that:
- Checks that each output window from random input has mean ≈ 0 and std ≈ 1 within a reasonable tolerance.
- Verifies that a constant-intensity window (std == 0) produces all zeros, matching the np.divide(..., where=(tmp != 0)) behavior.
- Optionally confirms that integer inputs (e.g. uint8) behave consistently with float inputs.
This will ensure the new normalization behavior is properly covered by tests.
</issue_to_address>
### Comment 4
<location path="openpiv/test/test_pyprocess.py" line_range="292" />
<code_context>
def test_fft_correlate_images():
</code_context>
<issue_to_address>
**suggestion (testing):** Add targeted tests for correlate_windows normalization and scaling by correlation field size
The new behavior normalizes `correlate_windows` by `corr.shape[-2] * corr.shape[-1]`, and `fft_correlate_images` now relies on `normalize_intensity` with zero-mean/unit-variance. The current tests only cover this indirectly.
Please add a focused test for `correlate_windows` that:
- Correlates identical windows of different sizes and asserts that the peak normalized correlation is comparable (i.e., does not scale with window area).
- Optionally verifies any flag that enables/disables normalization.
This will directly validate the new normalization and scaling behavior across window sizes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| print(f"correlation method {correlation_method } is not implemented") | ||
|
|
||
| return corr | ||
| return corr/(corr.shape[-2]*corr.shape[-1]) |
There was a problem hiding this comment.
issue (bug_risk): Consider whether correlate_windows should always normalize by window size or offer a flag for raw vs. normalized correlation.
This change always scales by window area, unlike fft_correlate_images, where normalization is controlled by normalized_correlation. If callers currently rely on raw correlation magnitudes, this becomes a backward-incompatible change and may cause subtle issues (e.g., double-normalization or misread peak heights). Consider adding a normalized_correlation parameter here too, or verifying that all call sites expect normalized output.
Comment on lines
755
to
756
| def normalize_intensity(window): | ||
| """Normalize interrogation window or strided image of many windows, |
There was a problem hiding this comment.
nitpick (typo): Fix typos and phrasing in the normalize_intensity docstring for clarity.
Use "standard deviation" and "might not be fully converged" here to improve clarity of the docstring.
Suggested change
| def normalize_intensity(window): | |
| """Normalize interrogation window or strided image of many windows, | |
| def normalize_intensity(window): | |
| """Normalize an interrogation window or a strided image of many windows | |
| by removing the mean intensity per window and dividing by the | |
| standard deviation. | |
| Note: for small signals the standard deviation might not be fully | |
| converged. NumPy also recommends using float64 for improved | |
| numerical accuracy: | |
| https://numpy.org/doc/stable/reference/generated/numpy.std.html |
Comment on lines
34
to
+43
| def test_normalize_intensity_performance(): | ||
| """Test that normalize_intensity avoids unnecessary conversions.""" | ||
| # Test with float32 input (should not convert) | ||
| window_float = np.random.rand(50, 64, 64).astype(np.float32) | ||
| # Test with float64 input (should not convert) | ||
| window_float = np.random.rand(50, 64, 64).astype(np.float64) | ||
|
|
||
| start = time.time() | ||
| result = pyprocess.normalize_intensity(window_float) | ||
| elapsed_float = time.time() - start | ||
|
|
||
| assert result.dtype == np.float32 | ||
| assert result.dtype == np.float64 |
There was a problem hiding this comment.
suggestion (testing): Add functional tests for normalize_intensity statistical properties and zero-variance windows
This test now only covers dtype and timing, but the function’s new guarantees (zero mean, unit variance, and zero-std handling) aren’t verified. Please add a separate, non-performance test (e.g. in test_pyprocess.py or a dedicated test_normalize_intensity.py) that:
- Checks that each output window from random input has mean ≈ 0 and std ≈ 1 within a reasonable tolerance.
- Verifies that a constant-intensity window (std == 0) produces all zeros, matching the np.divide(..., where=(tmp != 0)) behavior.
- Optionally confirms that integer inputs (e.g. uint8) behave consistently with float inputs.
This will ensure the new normalization behavior is properly covered by tests.
| @@ -290,7 +290,8 @@ def test_fft_correlate_images(): | |||
| assert corr_norm.shape[0] == 3 # Should have same batch size | |||
|
|
|||
| # Normalized correlation should have values between -1 and 1 | |||
There was a problem hiding this comment.
suggestion (testing): Add targeted tests for correlate_windows normalization and scaling by correlation field size
The new behavior normalizes correlate_windows by corr.shape[-2] * corr.shape[-1], and fft_correlate_images now relies on normalize_intensity with zero-mean/unit-variance. The current tests only cover this indirectly.
Please add a focused test for correlate_windows that:
- Correlates identical windows of different sizes and asserts that the peak normalized correlation is comparable (i.e., does not scale with window area).
- Optionally verifies any flag that enables/disables normalization.
This will directly validate the new normalization and scaling behavior across window sizes.
Member
|
@TKaeufer - have you tested these changes? |
Member
Author
|
It passed the automated tests, and I did some local tests. But did not do a full windef run. Could do that later.
…________________________________
Von: Alex Liberzon ***@***.***>
Gesendet: Dienstag, 21. April 2026 09:08
An: OpenPIV/openpiv-python ***@***.***>
Cc: Theo Käufer ***@***.***>; Mention ***@***.***>
Betreff: Re: [OpenPIV/openpiv-python] Fixed normalized cross correlation (PR #364)
[https://avatars.githubusercontent.com/u/747110?s=20&v=4]alexlib left a comment (OpenPIV/openpiv-python#364)<#364?email_source=notifications&email_token=ANHWT3T7EUXEJP4TS33FVUL4W5XEFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRYHA3TMMZXGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4288763765>
@TKaeufer<https://github.com/TKaeufer> - have you tested these changes?
—
Reply to this email directly, view it on GitHub<#364?email_source=notifications&email_token=ANHWT3T7EUXEJP4TS33FVUL4W5XEFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRYHA3TMMZXGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4288763765>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANHWT3UCQTSR5GI2UX6QKTL4W5XEFAVCNFSM6AAAAACYAHBCXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEOBYG43DGNZWGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
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
2 participants
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.
See issue: Intensity normalization #363
Summary by Sourcery
Update intensity normalization and normalized cross-correlation to use zero-mean, unit-variance windows and adjust correlation scaling accordingly.
Enhancements:
Tests: