Skip to content

Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss#8783

Open
ytl0623 wants to merge 4 commits intoProject-MONAI:devfrom
ytl0623:fix-issue-8780
Open

Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss#8783
ytl0623 wants to merge 4 commits intoProject-MONAI:devfrom
ytl0623:fix-issue-8780

Conversation

@ytl0623
Copy link
Contributor

@ytl0623 ytl0623 commented Mar 18, 2026

Fixes #8780

Description

Divide the pixel radius by sigma to convert it into the correct sigma-unit ratio.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

…rupted LocalNormalizedCrossCorrelationLoss

Signed-off-by: ytl0623 <david89062388@gmail.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a8759084-9863-4c68-a016-d383a8fefbda

📥 Commits

Reviewing files that changed from the base of the PR and between 8fa1130 and 7945c0a.

📒 Files selected for processing (1)
  • tests/integration/test_reg_loss_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/integration/test_reg_loss_integration.py

📝 Walkthrough

Walkthrough

Adjusted make_gaussian_kernel in monai/losses/image_dissimilarity.py to compute Gaussian truncation relative to sigma by using truncated = (kernel_size // 2) / sigma (retaining the multiplication by 2.5066282 * sigma and slicing to kernel_size). In tests/integration/test_reg_loss_integration.py, added a TEST_CASES entry for LocalNormalizedCrossCorrelationLoss with kernel_size=7 and kernel_type="gaussian", and added test_lncc_gaussian_kernel_gt3_identical_images which verifies LNCC on identical 2D inputs for kernel sizes 5 and 7 yields approximately -1.0.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive Description covers the fix (dividing pixel radius by sigma) and indicates non-breaking change, but doesn't explicitly mention new tests added despite TEST_CASES and test_lncc_gaussian_kernel_gt3_identical_images appearing in changes. Check the box 'New tests added to cover the changes' in the template to align description with the actual test additions included in this PR.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately describes the main fix: correcting the truncated parameter unit conversion in make_gaussian_kernel that corrupts LocalNormalizedCrossCorrelationLoss.
Linked Issues check ✅ Passed PR successfully addresses issue #8780 by fixing the unit mismatch: converting pixel radius to sigma units via truncated=(kernel_size // 2) / sigma, enabling correct Gaussian kernel generation and fixing LNCC for identical inputs.
Out of Scope Changes check ✅ Passed All changes are in scope: the kernel fix in image_dissimilarity.py and added tests in test_reg_loss_integration.py directly validate the fix for issue #8780.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ytl0623 ytl0623 changed the title Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss Mar 18, 2026
@ytl0623 ytl0623 changed the title Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss Mar 18, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
monai/losses/image_dissimilarity.py (1)

36-36: Add a Google-style docstring to make_gaussian_kernel.

This modified definition still lacks a docstring describing arguments, return value, and raised exceptions.

As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monai/losses/image_dissimilarity.py` at line 36, Add a Google-style docstring
to the make_gaussian_kernel function that documents the arguments, return value,
and any exceptions raised; specifically describe the kernel_size parameter (type
and expected constraints, e.g., positive odd integer), the return type
torch.Tensor (shape and contents: 2D Gaussian kernel normalized to sum 1), and
any ValueError raised for invalid kernel_size values. Place the docstring
immediately below the def make_gaussian_kernel(...) line in Google style with
Args:, Returns:, and Raises: sections so readers and autodoc tools can properly
parse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@monai/losses/image_dissimilarity.py`:
- Around line 38-41: Add a regression unit test that verifies
LocalNormalizedCrossCorrelationLoss returns ~-1.0 for identical images when
using gaussian kernels larger than 3: create a test that constructs
LocalNormalizedCrossCorrelationLoss(spatial_dims=2, kernel_size=5 or 7,
kernel_type="gaussian"), generate a random tensor x and set y = x.clone(),
compute loss = loss_fn(x,y) and assert torch.allclose(loss, torch.tensor(-1.0,
device=loss.device, dtype=loss.dtype), atol=1e-3); this ensures the gaussian_1d
-> kernel handling (the kernel variable and slicing logic that returns
kernel[:kernel_size]) behaves correctly for kernel_size > 3.

---

Nitpick comments:
In `@monai/losses/image_dissimilarity.py`:
- Line 36: Add a Google-style docstring to the make_gaussian_kernel function
that documents the arguments, return value, and any exceptions raised;
specifically describe the kernel_size parameter (type and expected constraints,
e.g., positive odd integer), the return type torch.Tensor (shape and contents:
2D Gaussian kernel normalized to sum 1), and any ValueError raised for invalid
kernel_size values. Place the docstring immediately below the def
make_gaussian_kernel(...) line in Google style with Args:, Returns:, and Raises:
sections so readers and autodoc tools can properly parse it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0f39dfb9-73ab-41e3-83a2-4fc08c4b2693

📥 Commits

Reviewing files that changed from the base of the PR and between daaedaa and 55a629c.

📒 Files selected for processing (1)
  • monai/losses/image_dissimilarity.py

Signed-off-by: ytl0623 <david89062388@gmail.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/integration/test_reg_loss_integration.py`:
- Around line 102-118: The test function
test_lncc_gaussian_kernel_gt3_identical_images is currently defined at module
scope and must be indented into the TestRegLossIntegration class so it runs as a
unittest method; move/indent the entire def
test_lncc_gaussian_kernel_gt3_identical_images(self): block into the
TestRegLossIntegration class body (maintain existing self references and
imports) so it becomes a method of TestRegLossIntegration and retains the
for-loop, subTest, and assertions unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 76bd2b90-dd87-43ad-a83c-f7186bfadb4e

📥 Commits

Reviewing files that changed from the base of the PR and between 55a629c and cfcee1f.

📒 Files selected for processing (1)
  • tests/integration/test_reg_loss_integration.py

ytl0623 and others added 2 commits March 19, 2026 09:18
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: ytl0623 <david89062388@gmail.com>
Signed-off-by: ytl0623 <david89062388@gmail.com>
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.

Bug in make_gaussian_kernel causes incorrect LocalNormalizedCrossCorrelationLoss when kernel_size > 3

1 participant