Fix align_corners mismatch in AffineTransform #8690
Conversation
📝 WalkthroughWalkthroughAffineTransform.forward now forwards Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
aymuos15
force-pushed
the
fix/affine-transform-align-corners
branch
from
945db18 to
2a33c89
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/transforms/lazy/utils.py (1)
104-110: Logic is sound; consider updating docstring.The defensive type checks are appropriate. The function's docstring could mention that
align_cornersis now extracted fromextra_infowhen present.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
monai/networks/layers/spatial_transforms.pymonai/transforms/lazy/utils.pytests/networks/layers/test_affine_transform.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/networks/layers/spatial_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/networks/layers/test_affine_transform.pymonai/transforms/lazy/utils.py
🧬 Code graph analysis (2)
tests/networks/layers/test_affine_transform.py (2)
tests/test_utils.py (1)
assert_allclose(119-159)monai/networks/layers/spatial_transforms.py (1)
AffineTransform(439-592)
monai/transforms/lazy/utils.py (2)
monai/utils/enums.py (2)
LazyAttr(646-660)TraceKeys(324-336)monai/utils/module.py (1)
look_up_option(61-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: build-docs
🔇 Additional comments (6)
monai/transforms/lazy/utils.py (1)
23-23: LGTM on import addition.
TraceKeysimport is appropriate for accessingEXTRA_INFO.tests/networks/layers/test_affine_transform.py (5)
153-158: Expected values align with the fix.With
align_corners=True(default), a 2x vertical zoom on a 3x4 image outputs the center row. Values [5, 6, 7, 8] are correct.
160-165: LGTM.2x zoom in both dimensions with
align_corners=Truesamples center values [5.0, 7.0].
167-172: LGTM.
zero_centered=Trueshifts the sampling origin. Expected [5.0, 8.0] reflects correct behavior.
383-406: Good regression test for the fix.Tests identity transform produces pixel-perfect output for both
align_cornerssettings. Directly validates the consistency fix.
407-428: Well-designed translation test.Validates pixel-space translation is correctly converted to normalized coordinates with
align_corners=True. Expected output is mathematically correct.
aymuos15
force-pushed
the
fix/affine-transform-align-corners
branch
from
c86bad4 to
7908302
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/networks/layers/test_affine_transform.py (1)
383-406: Good test for verifying align_corners consistency.The docstring clearly explains the test intent. Testing identity affine with both
align_corners=TrueandFalsevalidates thatto_norm_affinenow uses the same setting asaffine_grid/grid_sample.One minor suggestion: consider adding
dtype=torch.float32toidentity_affinefor explicitness, matching pattern used elsewhere in the file.Optional: explicit dtype
- identity_affine = torch.eye(3).unsqueeze(0) + identity_affine = torch.eye(3, dtype=torch.float32).unsqueeze(0)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
monai/networks/layers/spatial_transforms.pymonai/transforms/lazy/utils.pytests/networks/layers/test_affine_transform.pytests/transforms/test_affine.pytests/transforms/test_affined.py
🚧 Files skipped from review as they are similar to previous changes (3)
- monai/transforms/lazy/utils.py
- tests/transforms/test_affine.py
- monai/networks/layers/spatial_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/test_affined.pytests/networks/layers/test_affine_transform.py
🧬 Code graph analysis (2)
tests/transforms/test_affined.py (2)
monai/transforms/spatial/dictionary.py (1)
Affined(886-1015)tests/lazy_transforms_utils.py (1)
test_resampler_lazy(34-91)
tests/networks/layers/test_affine_transform.py (1)
monai/networks/layers/spatial_transforms.py (1)
AffineTransform(439-592)
🔇 Additional comments (5)
tests/networks/layers/test_affine_transform.py (4)
153-158: Updated expected values reflect the coordinate system fix.With
align_corners=True(default), a 2x zoom centered on a 3x4 image should sample row index 1 (the middle row). The middle row contains values 5, 6, 7, 8. The change from fractional values to clean integers confirms the fix eliminates the half-pixel offset.
160-165: Correct expected values for consistent align_corners behavior.For a 2x zoom in both dimensions on a 3x4 image with output size (1, 2), sampling the center pixels yields 5.0 and 7.0 when
align_corners=Trueis used consistently throughout the pipeline.
167-172: Zero-centered zoom expected values updated.With
zero_centered=True, the zoom is applied around the spatial center. The expected values 5.0 and 8.0 are correct for this configuration after the fix.
407-428: Translation test validates pixel-space conversion with align_corners=True.The test correctly verifies that a +1 pixel translation in the affine shifts sampling accordingly. The expected output (columns shifted left, rightmost becomes 0) is correct for
padding_mode="zeros".tests/transforms/test_affined.py (1)
179-186: Simplified lazy testing with consistent align_corners.Removing the per-iteration
align_cornersoverride ensures lazy and non-lazy paths use matching parameters. The comment adequately explains the reasoning.Note:
lazy_input_param = input_param.copy()creates a shallow copy. For this test case it's fine since no nested mutable objects are modified after copy.
The AffineTransform class was using inconsistent align_corners values: - to_norm_affine was hardcoded to use align_corners=False - affine_grid and grid_sample were using self.align_corners (default=True) This mismatch caused a half-pixel offset between coordinate systems, leading to incorrect spatial transformations. Changes: - Pass self.align_corners to to_norm_affine for consistent behavior - Update test expected values to reflect corrected behavior - Add test cases for align_corners consistency verification Fixes Project-MONAI#8688 Signed-off-by: Soumya Snigdha Kundu <[email protected]>
aymuos15
force-pushed
the
fix/affine-transform-align-corners
branch
from
7908302 to
cac7e8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/networks/layers/test_affine_transform.py:
- Around line 383-428: The new tests use .numpy() directly which is inconsistent
with the file's established pattern; update the three assertions in
test_align_corners_consistency and test_align_corners_true_translation to call
detach().cpu().numpy() on tensors before comparing: replace out_true.numpy(),
image.numpy(), out_false.numpy(), out.numpy(), and expected.numpy() with
out_true.detach().cpu().numpy(), image.detach().cpu().numpy(),
out_false.detach().cpu().numpy(), out.detach().cpu().numpy(), and
expected.detach().cpu().numpy() respectively so all tests follow the same
detach/cpu conversion pattern.
🧹 Nitpick comments (2)
tests/transforms/test_affine.py (2)
191-197: Lazy-vs-non-lazy check looks right; preferdeepcopyfor params to avoid shared mutable state.
Usinginput_param.copy()is shallow; if any nested values become mutable in future (e.g., arrays/tensors in params), this can create hard-to-debug coupling between the “regular” and “lazy” runs.Proposed tweak
- lazy_input_param = input_param.copy() + lazy_input_param = deepcopy(input_param) resampler = Affine(**lazy_input_param) non_lazy_result = resampler(**input_data) test_resampler_lazy(resampler, non_lazy_result, lazy_input_param, input_data, output_idx=output_idx)
239-242: Make the skip visible in test output (usesubTest+SkipTestinstead ofcontinue).
Right now CI will silently “pass” that combination; surfacing it as a skipped subtest is easier to track/regress.Proposed tweak
- for call in (method_0, method_1, method_2, method_3): - for ac in (False, True): - # Skip method_0 with align_corners=True due to known issue with lazy pipeline - # padding_mode override when using align_corners=True in optimized path - if call == method_0 and ac: - continue - out = call(im, ac) - ref = Resize(align_corners=ac, spatial_size=(sp_size, sp_size), mode="bilinear")(im) - assert_allclose(out, ref, rtol=1e-4, atol=1e-4, type_test=False) + for call in (method_0, method_1, method_2, method_3): + for ac in (False, True): + with self.subTest(method=call.__name__, align_corners=ac): + # Known issue: lazy pipeline padding_mode override when using align_corners=True in optimized path + if call is method_0 and ac: + raise unittest.SkipTest("Known lazy-path mismatch for method_0 with align_corners=True.") + out = call(im, ac) + ref = Resize(align_corners=ac, spatial_size=(sp_size, sp_size), mode="bilinear")(im) + assert_allclose(out, ref, rtol=1e-4, atol=1e-4, type_test=False)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
monai/networks/layers/spatial_transforms.pymonai/transforms/lazy/utils.pytests/networks/layers/test_affine_transform.pytests/transforms/test_affine.pytests/transforms/test_affined.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/transforms/test_affined.py
- monai/transforms/lazy/utils.py
- monai/networks/layers/spatial_transforms.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/networks/layers/test_affine_transform.pytests/transforms/test_affine.py
🧬 Code graph analysis (2)
tests/networks/layers/test_affine_transform.py (1)
monai/networks/layers/spatial_transforms.py (1)
AffineTransform(439-592)
tests/transforms/test_affine.py (2)
monai/transforms/lazy/utils.py (1)
Affine(28-45)tests/lazy_transforms_utils.py (1)
test_resampler_lazy(34-91)
🔇 Additional comments (1)
tests/networks/layers/test_affine_transform.py (1)
146-172: Updated zoom expectations match the new consistent align_corners behavior.
| def test_align_corners_consistency(self): | ||
| """ | ||
| Test that align_corners is consistently used between to_norm_affine and grid_sample. | ||
| With an identity affine transform, the output should match the input regardless of | ||
| the align_corners setting. This test verifies that the coordinate normalization | ||
| in to_norm_affine uses the same align_corners value as affine_grid/grid_sample. | ||
| """ | ||
| # Create a simple test image | ||
| image = torch.arange(1.0, 13.0).view(1, 1, 3, 4) | ||
|
|
||
| # Identity affine in pixel space (i, j, k convention with reverse_indexing=True) | ||
| identity_affine = torch.eye(3).unsqueeze(0) | ||
|
|
||
| # Test with align_corners=True (the default) | ||
| xform_true = AffineTransform(align_corners=True) | ||
| out_true = xform_true(image, identity_affine) | ||
| np.testing.assert_allclose(out_true.numpy(), image.numpy(), atol=1e-5, rtol=_rtol) | ||
|
|
||
| # Test with align_corners=False | ||
| xform_false = AffineTransform(align_corners=False) | ||
| out_false = xform_false(image, identity_affine) | ||
| np.testing.assert_allclose(out_false.numpy(), image.numpy(), atol=1e-5, rtol=_rtol) | ||
|
|
||
| def test_align_corners_true_translation(self): | ||
| """ | ||
| Test that translation works correctly with align_corners=True. | ||
| This ensures to_norm_affine correctly converts pixel-space translations | ||
| to normalized coordinates when align_corners=True. | ||
| """ | ||
| # 4x4 image | ||
| image = torch.arange(1.0, 17.0).view(1, 1, 4, 4) | ||
|
|
||
| # Translate by +1 pixel in the j direction (column direction) | ||
| # With reverse_indexing=True (default), this is the last spatial dimension | ||
| # Positive translation in the affine shifts the sampling grid, resulting in | ||
| # the output appearing shifted in the opposite direction | ||
| affine = torch.tensor([[[1.0, 0.0, 0.0], [0.0, 1.0, 1.0], [0.0, 0.0, 1.0]]]) | ||
|
|
||
| xform = AffineTransform(align_corners=True, padding_mode="zeros") | ||
| out = xform(image, affine) | ||
|
|
||
| # Expected: shift columns left by 1, rightmost column becomes 0 | ||
| expected = torch.tensor([[[[2, 3, 4, 0], [6, 7, 8, 0], [10, 11, 12, 0], [14, 15, 16, 0]]]], dtype=torch.float32) | ||
| np.testing.assert_allclose(out.numpy(), expected.numpy(), atol=1e-4, rtol=_rtol) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the test file to understand the context
wc -l tests/networks/layers/test_affine_transform.pyRepository: Project-MONAI/MONAI
Length of output: 113
🏁 Script executed:
# Look at the specific lines mentioned (383-428)
sed -n '383,428p' tests/networks/layers/test_affine_transform.pyRepository: Project-MONAI/MONAI
Length of output: 2378
🏁 Script executed:
# Search for .numpy() usage patterns in the entire test file to see what style is used elsewhere
rg "\.numpy\(\)" tests/networks/layers/test_affine_transform.py -nRepository: Project-MONAI/MONAI
Length of output: 1259
🏁 Script executed:
# Also check for detach().cpu().numpy() pattern
rg "detach\(\)\.cpu\(\)\.numpy\(\)" tests/networks/layers/test_affine_transform.py -nRepository: Project-MONAI/MONAI
Length of output: 975
🏁 Script executed:
# Check for any detach patterns
rg "detach\(\)" tests/networks/layers/test_affine_transform.py -nRepository: Project-MONAI/MONAI
Length of output: 975
Use detach().cpu().numpy() in the new tests to match the file's established pattern.
All other tests in this file (95, 105, 114, 120, 143, 180, 200, 217, 236, 260, 340+) use this pattern. The three new assertion lines (400, 405, 428) use .numpy() directly, creating inconsistency.
Proposed tweak
xform_true = AffineTransform(align_corners=True)
out_true = xform_true(image, identity_affine)
- np.testing.assert_allclose(out_true.numpy(), image.numpy(), atol=1e-5, rtol=_rtol)
+ np.testing.assert_allclose(out_true.detach().cpu().numpy(), image.detach().cpu().numpy(), atol=1e-5, rtol=_rtol)
# Test with align_corners=False
xform_false = AffineTransform(align_corners=False)
out_false = xform_false(image, identity_affine)
- np.testing.assert_allclose(out_false.numpy(), image.numpy(), atol=1e-5, rtol=_rtol)
+ np.testing.assert_allclose(out_false.detach().cpu().numpy(), image.detach().cpu().numpy(), atol=1e-5, rtol=_rtol)
...
xform = AffineTransform(align_corners=True, padding_mode="zeros")
out = xform(image, affine)
...
- np.testing.assert_allclose(out.numpy(), expected.numpy(), atol=1e-4, rtol=_rtol)
+ np.testing.assert_allclose(out.detach().cpu().numpy(), expected.detach().cpu().numpy(), atol=1e-4, rtol=_rtol)🤖 Prompt for AI Agents
In @tests/networks/layers/test_affine_transform.py around lines 383 - 428, The
new tests use .numpy() directly which is inconsistent with the file's
established pattern; update the three assertions in
test_align_corners_consistency and test_align_corners_true_translation to call
detach().cpu().numpy() on tensors before comparing: replace out_true.numpy(),
image.numpy(), out_false.numpy(), out.numpy(), and expected.numpy() with
out_true.detach().cpu().numpy(), image.detach().cpu().numpy(),
out_false.detach().cpu().numpy(), out.detach().cpu().numpy(), and
expected.detach().cpu().numpy() respectively so all tests follow the same
detach/cpu conversion pattern.
1 participant
Summary
align_cornersparameter inAffineTransformto_norm_affinecall was using hardcodedalign_corners=Falsewhileaffine_gridandgrid_sampleusedself.align_corners(defaultTrue), causing half-pixel offsetself.align_cornersconsistently across the coordinate transformation pipelineChanges
monai/networks/layers/spatial_transforms.pyto useself.align_cornersinstead of hardcodedFalsealign_cornersconsistencyTest Plan
align_corners=Trueandalign_corners=Falsealign_cornerssetting