Fix division by zero in Warp for singleton spatial dimensions#8946
Fix division by zero in Warp for singleton spatial dimensions#8946VenkateswarluNagineni wants to merge 2 commits into
Conversation
The native ``grid_sample`` path in ``Warp.forward`` normalizes grid coordinates with ``coord * 2 / (dim - 1) - 1``. When a spatial dimension has size 1 (e.g. a single-slice volume or a single-row/column image), ``dim - 1`` is 0 and the normalization divides by zero, producing ``inf``/``nan`` coordinates. With ``padding_mode="zeros"`` this turns the warped output into zeros (3D) or ``nan`` (2D) instead of returning the input under a zero displacement field. Clamp the denominator to 1 so the lone voxel maps to ``-1``, matching the guard already used by ``monai.networks.utils.normalize_transform`` (``norm[norm <= 1.0] = 2.0``). Non-singleton dimensions are unchanged. Add a regression test covering 2D and 3D single-slice inputs. Signed-off-by: VenkateswarluNagineni <venkates2002@tamu.edu>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/networks/blocks/warp/test_warp.py (1)
141-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a method docstring for the new test definition.
As per path instructions, "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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/networks/blocks/warp/test_warp.py` at line 141, The new test definition in test_singleton_spatial_dim is missing the required Google-style method docstring. Add a concise docstring to this test method describing what it verifies, including any relevant args, return value, and raised exceptions sections if applicable, and keep the style consistent with the other test methods in the class.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/networks/blocks/warp/test_warp.py`:
- Around line 146-150: The regression test in test_warp should explicitly
exercise the native grid_sample path instead of relying on environment-dependent
fallback behavior. Update the test around Warp(...) and its result call to force
the native branch using the existing Warp/test helper mechanism or a direct flag
on Warp so the checked cases always run through grid_sample regardless of
runtime setup.
---
Nitpick comments:
In `@tests/networks/blocks/warp/test_warp.py`:
- Line 141: The new test definition in test_singleton_spatial_dim is missing the
required Google-style method docstring. Add a concise docstring to this test
method describing what it verifies, including any relevant args, return value,
and raised exceptions sections if applicable, and keep the style consistent with
the other test methods in the class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 365796e2-2785-4f72-a3e4-26f2f4c1a718
📒 Files selected for processing (2)
monai/networks/blocks/warp.pytests/networks/blocks/warp/test_warp.py
Address review feedback: pin USE_COMPILED=False so the test always exercises the native grid_sample branch (the only path that normalizes the grid; the csrc grid_pull path is unaffected), and add a Google-style docstring. Signed-off-by: VenkateswarluNagineni <venkates2002@tamu.edu>
|
Thanks for the review. Addressed both points in the latest commit:
Verified locally: the test fails on the pre-fix code and passes with the fix; |
Description
Warp.forward(nativegrid_samplepath) normalizes grid coordinates with:When a spatial dimension has size 1 — a single-slice volume
(B, C, 1, H, W)or a single-row/column image(B, C, 1, W)/(B, C, H, 1)—dim - 1 == 0, so the normalization divides by zero and producesinf/nancoordinates. Withpadding_mode="zeros", even a zero displacement field then yields:nanoutput for 2D single-row/column inputs, andinstead of returning the input image unchanged.
Fix
Clamp the denominator to 1 so the lone voxel maps to
-1. This mirrors the guard MONAI already applies inmonai.networks.utils.normalize_transform, which performs the identicalalign_corners=Truenormalization and clamps the size withnorm[norm <= 1.0] = 2.0to avoid exactly this division by zero. Non-singleton dimensions are numerically unchanged.Verification
Added
test_singleton_spatial_dim(2D single-row, 2D single-column, 3D single-slice). With a zero displacement field the warped output now equals the input and contains nonan. The test fails on the current code and passes with the fix; the existingWarptests are unaffected.Types of changes
./runtests.sh --quick --unittests --disttests.