Fix align_corners mismatch in AffineTransform#8690
Fix align_corners mismatch in AffineTransform#8690aymuos15 wants to merge 11 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
|
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.
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
| # 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? |
|
Reviewing now |
|
Hi @atbenmurray Do you think there is anything else left to do here? thanks :D |
atbenmurray
left a comment
atbenmurray
left a comment
There was a problem hiding this comment.
Can you address the one comment? It might not need a change
As pointed out by @atbenmurray in review, when Spacing.__call__ is invoked without an explicit align_corners, the resampler resolves None to self.sp_resample.align_corners. The recompute_affine path was hardcoding False, which caused the recomputed affine to diverge from the actual sampling when Spacing was constructed with align_corners=True. Fetch the effective value from sp_resample so metadata matches the resampling behavior. Update test_inverse_mn_mx expected pixdim to reflect the corrected align_corners=True scaling (orig-1)/(new-1). Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
|
@atbenmurray thank you very much for pointing that out. That had slipped me. Have fixed it now properly |
|
Hi @aymuos15 please check the Coderabbit comments to see if there's anything to address then marked as resolved. With that, if @atbenmurray can approve we can merge this soon. |
- scale_affine: handle dst dim == 1 special case (avoid spurious non-zero scale via max(n-1, 1)) and document that `centered` is ignored when align_corners=True. - kwargs_from_pending: docstring now notes align_corners extraction from extra_info. - test_affine_transform: use detach().cpu().numpy() consistently and pin identity_affine dtype. - test_affine: switch the known method_0/align_corners=True mismatch to subTest + SkipTest so it stays visible in CI; deepcopy lazy params. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Everything should be addressed now, thank you! |
3 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