Fix multi-axis shear transform to compose individual shear matrices#8778
Fix multi-axis shear transform to compose individual shear matrices#8778aymuos15 wants to merge 3 commits intoProject-MONAI:devfrom
Conversation
…roject-MONAI#8450) The shear matrix was constructed by placing all coefficients into a single identity matrix, producing a non-area-preserving transform (determinant != 1) when multiple axes were sheared simultaneously. This also meant that applying shears sequentially gave different results from applying them all at once. Fix by composing individual single-axis shear matrices via matrix multiplication, which guarantees determinant = 1 and sequential equivalence. Update hardcoded test expectations to match the corrected math. Fixes Project-MONAI#8450 Project-MONAI#8452 Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
📝 WalkthroughWalkthroughRefactors internal shear matrix construction in monai/transforms/utils.py to compose elementary single-axis shear matrices via successive matrix multiplications instead of setting multiple off-diagonal entries directly. 2D now composes two rank-3 shears; 3D composes six elementary shears in a loop. Public API and error behavior unchanged. Tests updated: adjusted expected matrices for RandAffine/RandAffined outputs and added/updated tests verifying determinant ≈1 and sequential-equivalence of composed shears. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Thanks @aymuos15 I think this looks fine, I'd like @atbenmurray to have a look as well if possible. We need to merge a fix shortly then can come back to this one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@monai/transforms/utils.py`:
- Around line 958-966: The Affine/AffineGrid documentation still describes
shear_params as direct off-diagonal assignments; update the Affine/AffineGrid
docstring (the Affine class and/or AffineGrid transform) to match the
composed-shear behavior: state that shear_params are composed shearing factors
(tuple of 2 floats for 2D, tuple of 6 for 3D), explain the multiplication order
used to compose single-axis shear matrices into the final affine (and that this
yields determinant 1), and give the same example matrix form (e.g., for 2D (Sx,
Sy) produce the composed matrix shown in the shear utils doc) so users
understand how parameters map to the resulting affine.
🪄 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: a3776a96-a3fc-41e5-bfaa-ba25d25359b4
📒 Files selected for processing (1)
monai/transforms/utils.py
| coefs: shearing factors, a tuple of 2 floats for 2D, a tuple of 6 floats for 3D). | ||
| Individual single-axis shear matrices are composed (multiplied) in | ||
| coefficient order so that the result is a proper shear with determinant 1. | ||
| For 2D with coefs ``(Sx, Sy)`` the composed matrix is:: | ||
|
|
||
| [ | ||
| [1.0, coefs[0], coefs[1], 0.0], | ||
| [coefs[2], 1.0, coefs[3], 0.0], | ||
| [coefs[4], coefs[5], 1.0, 0.0], | ||
| [0.0, 0.0, 0.0, 1.0], | ||
| [1.0, Sx, 0.0], | ||
| [Sy, 1.0 + Sx*Sy, 0.0], | ||
| [0.0, 0.0, 1.0], |
There was a problem hiding this comment.
Update the related Affine shear docs too.
This doc now describes composed shears, but monai/transforms/spatial/array.py:2160-2195 still documents shear_params as direct off-diagonal assignment. That will mislead users of Affine/AffineGrid.
Docs follow-up
- shear_params: shearing factors for affine matrix, take a 3D affine as example::
+ shear_params: shearing factors for the composed shear matrix. For 3D, the six coefficients are applied
+ in coefficient order as elementary single-axis shears.As per coding guidelines, “Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/transforms/utils.py` around lines 958 - 966, The Affine/AffineGrid
documentation still describes shear_params as direct off-diagonal assignments;
update the Affine/AffineGrid docstring (the Affine class and/or AffineGrid
transform) to match the composed-shear behavior: state that shear_params are
composed shearing factors (tuple of 2 floats for 2D, tuple of 6 for 3D), explain
the multiplication order used to compose single-axis shear matrices into the
final affine (and that this yields determinant 1), and give the same example
matrix form (e.g., for 2D (Sx, Sy) produce the composed matrix shown in the
shear utils doc) so users understand how parameters map to the resulting affine.
2 participants
Fix Shear transform