Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss#8783
Open
ytl0623 wants to merge 2 commits intoProject-MONAI:devfrom
Open
Fix incorrect truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss#8783ytl0623 wants to merge 2 commits intoProject-MONAI:devfrom
truncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss#8783ytl0623 wants to merge 2 commits intoProject-MONAI:devfrom
Conversation
…rupted LocalNormalizedCrossCorrelationLoss Signed-off-by: ytl0623 <david89062388@gmail.com>
Contributor
📝 WalkthroughWalkthroughUpdated make_gaussian_kernel in monai/losses/image_dissimilarity.py to compute the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip Flake8 can be used to improve the quality of Python code reviews.Flake8 is a Python linter that wraps PyFlakes, pycodestyle and Ned Batchelder's McCabe script. To configure Flake8, add a '.flake8' or 'setup.cfg' file to your project root. See Flake8 Documentation for more details. |
ytl0623
changed the title
make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss
ytl0623
changed the title
make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLosstruncated parameter in make_gaussian_kernel causing corrupted LocalNormalizedCrossCorrelationLoss
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/losses/image_dissimilarity.py (1)
36-36: Add a Google-style docstring tomake_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
📒 Files selected for processing (1)
monai/losses/image_dissimilarity.py
Signed-off-by: ytl0623 <david89062388@gmail.com>
Contributor
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
tests/integration/test_reg_loss_integration.py
Comment on lines
+102
to
+118
| def test_lncc_gaussian_kernel_gt3_identical_images(self): | ||
| """ | ||
| Regression test for make_gaussian_kernel truncated parameter bug. | ||
| LNCC on identical inputs must be close to -1.0 for gaussian kernel_size > 3. | ||
| """ | ||
| for kernel_size in [5, 7]: | ||
| with self.subTest(kernel_size=kernel_size): | ||
| loss_fn = LocalNormalizedCrossCorrelationLoss( | ||
| spatial_dims=2, kernel_size=kernel_size, kernel_type="gaussian" | ||
| ).to(self.device) | ||
| x = torch.rand(2, 1, 32, 32, device=self.device) | ||
| y = x.clone() | ||
| loss = loss_fn(x, y) | ||
| self.assertTrue( | ||
| torch.allclose(loss, torch.tensor(-1.0, device=self.device, dtype=loss.dtype), atol=1e-3), | ||
| f"LNCC of identical images should be -1.0, got {loss.item():.6f} (kernel_size={kernel_size})", | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/test_reg_loss_integration.py"
echo "=== Structural context ==="
rg -n -C2 '^(class TestRegLossIntegration|def test_lncc_gaussian_kernel_gt3_identical_images| def test_lncc_gaussian_kernel_gt3_identical_images)' "$FILE"
echo
echo "=== AST owner check ==="
python - <<'PY'
import ast
from pathlib import Path
path = Path("tests/integration/test_reg_loss_integration.py")
tree = ast.parse(path.read_text())
owner = "not found"
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "test_lncc_gaussian_kernel_gt3_identical_images":
owner = "module"
if isinstance(node, ast.ClassDef) and node.name == "TestRegLossIntegration":
for sub in node.body:
if isinstance(sub, ast.FunctionDef) and sub.name == "test_lncc_gaussian_kernel_gt3_identical_images":
owner = "TestRegLossIntegration"
print(f"owner={owner}")
PYRepository: Project-MONAI/MONAI
Length of output: 482
Indent test method into TestRegLossIntegration class.
Function at line 102 is module-scoped and won't execute as a unittest method. Apply indentation to move it inside the class body.
Proposed fix
-def test_lncc_gaussian_kernel_gt3_identical_images(self):
- """
- Regression test for make_gaussian_kernel truncated parameter bug.
- LNCC on identical inputs must be close to -1.0 for gaussian kernel_size > 3.
- """
- for kernel_size in [5, 7]:
- with self.subTest(kernel_size=kernel_size):
- loss_fn = LocalNormalizedCrossCorrelationLoss(
- spatial_dims=2, kernel_size=kernel_size, kernel_type="gaussian"
- ).to(self.device)
- x = torch.rand(2, 1, 32, 32, device=self.device)
- y = x.clone()
- loss = loss_fn(x, y)
- self.assertTrue(
- torch.allclose(loss, torch.tensor(-1.0, device=self.device, dtype=loss.dtype), atol=1e-3),
- f"LNCC of identical images should be -1.0, got {loss.item():.6f} (kernel_size={kernel_size})",
- )
+ def test_lncc_gaussian_kernel_gt3_identical_images(self):
+ """
+ Regression test for make_gaussian_kernel truncated parameter bug.
+ LNCC on identical inputs must be close to -1.0 for gaussian kernel_size > 3.
+ """
+ for kernel_size in [5, 7]:
+ with self.subTest(kernel_size=kernel_size):
+ loss_fn = LocalNormalizedCrossCorrelationLoss(
+ spatial_dims=2, kernel_size=kernel_size, kernel_type="gaussian"
+ ).to(self.device)
+ x = torch.rand(2, 1, 32, 32, device=self.device)
+ y = x.clone()
+ loss = loss_fn(x, y)
+ self.assertTrue(
+ torch.allclose(loss, torch.tensor(-1.0, device=self.device, dtype=loss.dtype), atol=1e-3),
+ f"LNCC of identical images should be -1.0, got {loss.item():.6f} (kernel_size={kernel_size})",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_lncc_gaussian_kernel_gt3_identical_images(self): | |
| """ | |
| Regression test for make_gaussian_kernel truncated parameter bug. | |
| LNCC on identical inputs must be close to -1.0 for gaussian kernel_size > 3. | |
| """ | |
| for kernel_size in [5, 7]: | |
| with self.subTest(kernel_size=kernel_size): | |
| loss_fn = LocalNormalizedCrossCorrelationLoss( | |
| spatial_dims=2, kernel_size=kernel_size, kernel_type="gaussian" | |
| ).to(self.device) | |
| x = torch.rand(2, 1, 32, 32, device=self.device) | |
| y = x.clone() | |
| loss = loss_fn(x, y) | |
| self.assertTrue( | |
| torch.allclose(loss, torch.tensor(-1.0, device=self.device, dtype=loss.dtype), atol=1e-3), | |
| f"LNCC of identical images should be -1.0, got {loss.item():.6f} (kernel_size={kernel_size})", | |
| ) | |
| def test_lncc_gaussian_kernel_gt3_identical_images(self): | |
| """ | |
| Regression test for make_gaussian_kernel truncated parameter bug. | |
| LNCC on identical inputs must be close to -1.0 for gaussian kernel_size > 3. | |
| """ | |
| for kernel_size in [5, 7]: | |
| with self.subTest(kernel_size=kernel_size): | |
| loss_fn = LocalNormalizedCrossCorrelationLoss( | |
| spatial_dims=2, kernel_size=kernel_size, kernel_type="gaussian" | |
| ).to(self.device) | |
| x = torch.rand(2, 1, 32, 32, device=self.device) | |
| y = x.clone() | |
| loss = loss_fn(x, y) | |
| self.assertTrue( | |
| torch.allclose(loss, torch.tensor(-1.0, device=self.device, dtype=loss.dtype), atol=1e-3), | |
| f"LNCC of identical images should be -1.0, got {loss.item():.6f} (kernel_size={kernel_size})", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_reg_loss_integration.py` around lines 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.
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
1 participant
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 #8780
Description
Divide the pixel radius by
sigmato convert it into the correct sigma-unit ratio.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.