Fix align_corners mismatch in AffineTransform#8690
Fix align_corners mismatch in AffineTransform#8690aymuos15 wants to merge 5 commits intoProject-MONAI:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request propagates the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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🧪 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 |
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 <soumya_snigdha.kundu@kcl.ac.uk>
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.
|
Hi, taking a look at this |
The lazy and non-lazy Zoom transform paths produced different results when align_corners=True because scale_affine() always used src/dst scaling regardless of align_corners setting. Changes: - Add align_corners parameter to scale_affine() function - For align_corners=True: use (src-1)/(dst-1) scaling formula - For align_corners=True: skip centered offset (inherently centered) - Pass align_corners to scale_affine() in zoom(), resize(), and Spacing The key insight is that for align_corners=True, the scaling is inherently centered because corner pixels map to corner pixels, so no additional offset is needed. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@monai/transforms/spatial/array.py`:
- Around line 543-545: The code currently forces ac = False when align_corners
is None; instead resolve the effective default from the Spacing metadata and use
that when computing the affine. Replace the ac assignment with something like ac
= align_corners if align_corners is not None else spacing.align_corners (or
self.spacing.align_corners / the object that stores the Spacing config), then
call scale_affine(..., align_corners=ac) and continue assigning
data_array.affine via convert_to_dst_type(a, affine_)[0]; this ensures
recompute_affine (scale_affine + convert_to_dst_type) uses the same effective
align_corners as the Spacing object.
🧹 Nitpick comments (1)
monai/transforms/utils.py (1)
2098-2120: Document thatcenteredis ignored whenalign_corners=True.Right now
centeredhas no effect in that branch; a brief note in the docstring would prevent ambiguity.
| ac = align_corners if align_corners is not None else False | ||
| a = scale_affine(original_spatial_shape, actual_shape, align_corners=ac) | ||
| data_array.affine = convert_to_dst_type(a, affine_)[0] # type: ignore |
There was a problem hiding this comment.
Align recompute_affine with the effective align_corners.
If align_corners is None, this forces False even when Spacing was created with align_corners=True, so metadata can diverge from actual resampling. Use the resolved default.
🐛 Proposed fix
- ac = align_corners if align_corners is not None else False
- a = scale_affine(original_spatial_shape, actual_shape, align_corners=ac)
+ ac = self.sp_resample.align_corners if align_corners is None else align_corners
+ a = scale_affine(original_spatial_shape, actual_shape, align_corners=ac)🤖 Prompt for AI Agents
In `@monai/transforms/spatial/array.py` around lines 543 - 545, The code currently
forces ac = False when align_corners is None; instead resolve the effective
default from the Spacing metadata and use that when computing the affine.
Replace the ac assignment with something like ac = align_corners if
align_corners is not None else spacing.align_corners (or
self.spacing.align_corners / the object that stores the Spacing config), then
call scale_affine(..., align_corners=ac) and continue assigning
data_array.affine via convert_to_dst_type(a, affine_)[0]; this ensures
recompute_affine (scale_affine + convert_to_dst_type) uses the same effective
align_corners as the Spacing object.
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
aymuos15
force-pushed
the
fix/affine-transform-align-corners
branch
from
a8d83d3 to
86dc018
Compare
|
Sorry for the delay. On it now |
Resolve merge conflicts in test_affine.py and test_affined.py by accepting dev's loop over both align_corners values in lazy tests. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
monai/transforms/spatial/array.py (1)
543-544:⚠️ Potential issue | 🟠 MajorUse the effective
align_cornersdefault here.Line 543 hardcodes
Falsewhenalign_cornersisNone, but actual resampling resolvesNonetoself.sp_resample.align_corners. This can desync recomputed affine metadata from the real sampling behavior.As per coding guidelines "Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these."🐛 Proposed fix
- ac = align_corners if align_corners is not None else False + ac = self.sp_resample.align_corners if align_corners is None else align_corners a = scale_affine(original_spatial_shape, actual_shape, align_corners=ac)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/spatial/array.py` around lines 543 - 544, The code currently hardcodes align_corners=False when align_corners is None, causing affine metadata to diverge from actual resampling; update the logic to use the effective align_corners resolution used by the resampler (i.e., effective = align_corners if align_corners is not None else self.sp_resample.align_corners) and pass that effective value into scale_affine(original_spatial_shape, actual_shape, align_corners=effective) so the recomputed affine matches the real sampling behavior.
🤖 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/transforms/utils.py`:
- Around line 2102-2109: The align_corners branch incorrectly uses max(n - 1,
1), producing a non-zero scale when a destination dimension is 1; change the
calculation in the block that sets s so that when align_corners is True and a
new spatial size element n == 1 the scale for that dimension is set to 0 (i.e.,
handle the n==1 special-case explicitly instead of using max), updating the
computation that builds s (the list comprehension referencing spatial_size and
new_spatial_size) accordingly; also add regression tests covering
new_spatial_size containing 1 for both align_corners=True and
align_corners=False to ensure affine metadata and behavior match PyTorch
interpolate.
In `@tests/transforms/test_affine.py`:
- Around line 241-244: The test currently silently skips the case where call ==
method_0 and ac (align_corners=True); instead of continue, add an explicit
expected-failure test (or mark the specific case with pytest.mark.xfail) so the
failing path remains visible in CI and links to the relevant issue/PR; update
the test harness around method_0/align_corners to assert xfail with a clear
reason (and issue reference) or create a separate test function for method_0
with align_corners=True that is decorated as xfail so coverage remains and the
failure is tracked.
---
Duplicate comments:
In `@monai/transforms/spatial/array.py`:
- Around line 543-544: The code currently hardcodes align_corners=False when
align_corners is None, causing affine metadata to diverge from actual
resampling; update the logic to use the effective align_corners resolution used
by the resampler (i.e., effective = align_corners if align_corners is not None
else self.sp_resample.align_corners) and pass that effective value into
scale_affine(original_spatial_shape, actual_shape, align_corners=effective) so
the recomputed affine matches the real sampling behavior.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48288c3a-02db-4372-9403-6aba8f535988
📒 Files selected for processing (3)
monai/transforms/spatial/array.pymonai/transforms/utils.pytests/transforms/test_affine.py
| if align_corners: | ||
| # Match interpolate behavior: (src-1)/(dst-1) | ||
| s = np.array( | ||
| [(float(o) - 1) / max(float(n) - 1, 1) for o, n in zip(spatial_size, new_spatial_size)], dtype=float | ||
| ) | ||
| else: | ||
| # Standard scaling: src/dst | ||
| s = np.array([float(o) / float(max(n, 1)) for o, n in zip(spatial_size, new_spatial_size)], dtype=float) |
There was a problem hiding this comment.
Handle align_corners=True correctly when destination size is 1.
Line 2105 currently uses max(n - 1, 1), which yields a non-zero scale for n == 1. That diverges from PyTorch interpolate’s special-case behavior and can produce incorrect affine metadata for collapsed dimensions.
Suggested patch
- if align_corners:
- # Match interpolate behavior: (src-1)/(dst-1)
- s = np.array(
- [(float(o) - 1) / max(float(n) - 1, 1) for o, n in zip(spatial_size, new_spatial_size)], dtype=float
- )
+ if align_corners:
+ # Match interpolate behavior: (src-1)/(dst-1), with dst==1 special-cased to 0.
+ s = np.array(
+ [
+ 0.0 if float(n) <= 1.0 else (float(o) - 1.0) / (float(n) - 1.0)
+ for o, n in zip(spatial_size, new_spatial_size, strict=False)
+ ],
+ dtype=float,
+ )
else:
# Standard scaling: src/dst
- s = np.array([float(o) / float(max(n, 1)) for o, n in zip(spatial_size, new_spatial_size)], dtype=float)
+ s = np.array(
+ [float(o) / float(max(n, 1)) for o, n in zip(spatial_size, new_spatial_size, strict=False)],
+ dtype=float,
+ )Also add a regression test for new_spatial_size containing 1 with both align_corners=True/False.
In official PyTorch docs/source for torch.nn.functional.interpolate, what is the coordinate mapping behavior when align_corners=True and output size is 1 (linear/bilinear/trilinear modes)?
As per coding guidelines "Ensure new or modified definitions will be covered by existing or new unit tests."
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 2105-2105: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 2109-2109: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/transforms/utils.py` around lines 2102 - 2109, The align_corners branch
incorrectly uses max(n - 1, 1), producing a non-zero scale when a destination
dimension is 1; change the calculation in the block that sets s so that when
align_corners is True and a new spatial size element n == 1 the scale for that
dimension is set to 0 (i.e., handle the n==1 special-case explicitly instead of
using max), updating the computation that builds s (the list comprehension
referencing spatial_size and new_spatial_size) accordingly; also add regression
tests covering new_spatial_size containing 1 for both align_corners=True and
align_corners=False to ensure affine metadata and behavior match PyTorch
interpolate.
| # 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 |
There was a problem hiding this comment.
Avoid silently skipping method_0 with align_corners=True.
Line 243 drops coverage for a known failing path in the exact area this PR changes. Please keep this case visible (e.g., dedicated expected-failure test with issue tracking) instead of a silent continue.
As per coding guidelines "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/transforms/test_affine.py` around lines 241 - 244, The test currently
silently skips the case where call == method_0 and ac (align_corners=True);
instead of continue, add an explicit expected-failure test (or mark the specific
case with pytest.mark.xfail) so the failing path remains visible in CI and links
to the relevant issue/PR; update the test harness around method_0/align_corners
to assert xfail with a clear reason (and issue reference) or create a separate
test function for method_0 with align_corners=True that is decorated as xfail so
coverage remains and the failure is tracked.
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
|
I fixed the merge conflicts. Anything else to do here? |
2 participants
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