Improve Affine transform documentation and add compute_w_affine tests#8727
Improve Affine transform documentation and add compute_w_affine tests#8727engmohamedsalah wants to merge 1 commit intoProject-MONAI:devfrom
Conversation
engmohamedsalah
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
|
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:
📝 WalkthroughWalkthroughAdded a public classmethod Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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
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/array.py (1)
2333-2358:⚠️ Potential issue | 🟡 MinorAdd focused unit test for
compute_w_affineand preserve input type.The method lacks direct test coverage. Add a focused test (e.g., 2D case with known center shifts). Additionally, align with MONAI patterns by preserving the input matrix type:
Type preservation refactor
- mat = shift_1 @ convert_data_type(mat, np.ndarray)[0] @ shift_2 - return mat + mat_np = shift_1 @ convert_data_type(mat, np.ndarray)[0] @ shift_2 + return convert_to_dst_type(mat_np, mat)[0]
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/transforms/test_affine.py (2)
220-228: Partial assertion +zipwithoutstrict=.Two issues in
test_different_sizes:
result[:2, 2]only verifies the translation column; the upper-left 2×2 sub-matrix (should benp.eye(2)for an identitymat) is never asserted. A full matrix comparison would be more robust.zip(img_size, sp_size)silently truncates if lengths differ — addstrict=True.♻️ Proposed fix
mat = np.eye(3) img_size = (4, 4) sp_size = (8, 8) result = Affine.compute_w_affine(2, mat, img_size, sp_size) # Translation should account for the shift: (4-1)/2 - (8-1)/2 = 1.5 - 3.5 = -2.0 - expected_translation = np.array([(d1 - 1) / 2 - (d2 - 1) / 2 for d1, d2 in zip(img_size, sp_size)]) - assert_allclose(result[:2, 2], expected_translation, atol=1e-6) + expected_translation = np.array([(d1 - 1) / 2 - (d2 - 1) / 2 for d1, d2 in zip(img_size, sp_size, strict=True)]) + expected = np.eye(3) + expected[:2, 2] = expected_translation + assert_allclose(result, expected, atol=1e-6)As per coding guidelines, ensure logic errors and edge-case inconsistencies are addressed.
🤖 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 220 - 228, The test_different_sizes currently only asserts the translation column and uses zip(img_size, sp_size) without strict checking; update the test to construct the full expected 3x3 affine matrix (upper-left 2x2 should be np.eye(2) for identity mat and bottom row [0,0,1]) with the translation computed via zip(img_size, sp_size, strict=True), then assert_allclose(result, expected_matrix, atol=1e-6) against Affine.compute_w_affine's returned result to verify both rotation/scale and translation.
202-241: No test covers a non-identitymat.Every test passes
np.eye(n)asmat, so the centering compositionshift_1 @ R @ shift_2is never exercised for a rotation or scale. The most important behaviour ofcompute_w_affine(that a non-trivial transform is correctly sandwiched between center-shifts) is untested.A minimal addition — e.g. a 90° rotation in 2D with known expected output — would close this gap.
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 202 - 241, Add a new unit test in tests/transforms/test_affine.py that passes a non-identity transform to Affine.compute_w_affine to exercise the centering composition; for example create a 2D 90° rotation matrix (use numpy or torch), call Affine.compute_w_affine(2, rot, img_size, sp_size) with equal img_size and sp_size (so center shifts cancel), and assert the result equals the pure rotation (use assert_allclose with atol). Name the test e.g. test_rotation_90_2d and reuse the existing patterns (np.eye tests) to keep shape and tolerance checks consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/transforms/test_affine.py`:
- Around line 220-228: The test_different_sizes currently only asserts the
translation column and uses zip(img_size, sp_size) without strict checking;
update the test to construct the full expected 3x3 affine matrix (upper-left 2x2
should be np.eye(2) for identity mat and bottom row [0,0,1]) with the
translation computed via zip(img_size, sp_size, strict=True), then
assert_allclose(result, expected_matrix, atol=1e-6) against
Affine.compute_w_affine's returned result to verify both rotation/scale and
translation.
- Around line 202-241: Add a new unit test in tests/transforms/test_affine.py
that passes a non-identity transform to Affine.compute_w_affine to exercise the
centering composition; for example create a 2D 90° rotation matrix (use numpy or
torch), call Affine.compute_w_affine(2, rot, img_size, sp_size) with equal
img_size and sp_size (so center shifts cancel), and assert the result equals the
pure rotation (use assert_allclose with atol). Name the test e.g.
test_rotation_90_2d and reuse the existing patterns (np.eye tests) to keep shape
and tolerance checks consistent.
ℹ️ Review info
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 (1)
tests/transforms/test_affine.py
engmohamedsalah
force-pushed
the
improve-affine-docs-7092
branch
from
f08fa24 to
7bbcf4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/transforms/test_affine.py (1)
220-228:test_different_sizesonly validates the translation column — consider asserting the full matrix.With
mat=np.eye(3), the linear part stays identity and the expected full result is a pure translation matrix. Assertingresult[:2, 2]alone leaves the rotation/scale block untested.♻️ Proposed refactor
def test_different_sizes(self): """When img_size != sp_size, result should include net translation.""" mat = np.eye(3) img_size = (4, 4) sp_size = (8, 8) result = Affine.compute_w_affine(2, mat, img_size, sp_size) - # Translation should account for the shift: (4-1)/2 - (8-1)/2 = 1.5 - 3.5 = -2.0 - expected_translation = np.array([(d1 - 1) / 2 - (d2 - 1) / 2 for d1, d2 in zip(img_size, sp_size)]) - assert_allclose(result[:2, 2], expected_translation, atol=1e-6) + # (4-1)/2 - (8-1)/2 = 1.5 - 3.5 = -2.0 along each axis + expected = np.array([[1.0, 0.0, -2.0], [0.0, 1.0, -2.0], [0.0, 0.0, 1.0]]) + assert_allclose(result, expected, atol=1e-6)🤖 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 220 - 228, Update the test_different_sizes to assert the entire affine matrix returned by Affine.compute_w_affine (called with mat, img_size, sp_size) rather than only the translation column: construct the expected full 3x3 translation matrix (identity in the upper-left 2x2, expected_translation in result[:2,2], and [0,0,1] as the last row) and replace the single-column assert with an assertion that result equals this expected matrix (use the same assert_allclose with atol=1e-6).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/transforms/test_affine.py`:
- Line 227: Update the zip call in the expected_translation assignment to make
intent explicit by adding strict=False; change the comprehension using
zip(img_size, sp_size) in tests/transforms/test_affine.py (the
expected_translation expression) to use zip(img_size, sp_size, strict=False) so
Ruff B905 is satisfied and the equal-length assumption is explicit.
---
Nitpick comments:
In `@tests/transforms/test_affine.py`:
- Around line 220-228: Update the test_different_sizes to assert the entire
affine matrix returned by Affine.compute_w_affine (called with mat, img_size,
sp_size) rather than only the translation column: construct the expected full
3x3 translation matrix (identity in the upper-left 2x2, expected_translation in
result[:2,2], and [0,0,1] as the last row) and replace the single-column assert
with an assertion that result equals this expected matrix (use the same
assert_allclose with atol=1e-6).
ℹ️ Review info
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 (1)
tests/transforms/test_affine.py
engmohamedsalah
force-pushed
the
improve-affine-docs-7092
branch
from
7bbcf4e to
70e1978
Compare
engmohamedsalah
changed the title
- Add Note section documenting center-origin coordinate system assumption - Clarify normalized parameter documentation with user-friendly explanation - Add comprehensive docstring to compute_w_affine method - Add focused unit tests for compute_w_affine (2D/3D identity, different sizes, output shape, torch input compatibility) Fixes Project-MONAI#7092 Signed-off-by: Mohamed Salah <eng.mohamed.tawab@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/transforms/test_affine.py (1)
227-227:zip()missing explicitstrict=.Ruff B905 flags this; same issue previously raised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transforms/test_affine.py` at line 227, The list comprehension assigning expected_translation uses zip(img_size, sp_size) without the strict flag; update that expression in tests/transforms/test_affine.py (the expected_translation assignment) to call zip(img_size, sp_size, strict=True) so the test enforces equal-length iterables (keeping the rest of the list comprehension intact).
🧹 Nitpick comments (1)
monai/transforms/spatial/array.py (1)
2334-2358:Returns:should specify type and shape; method signature missing type annotations.The docstring addition is welcome, but the
Returns:entry doesn't state the concrete type or the resulting matrix shape, both of which are easily derivable from the body:- Returns: - The adjusted affine matrix that can be applied to image coordinates. + Returns: + np.ndarray: The adjusted (r+1) x (r+1) affine matrix expressed in + corner-based image coordinates.Also, the method signature (line 2334) still lacks type annotations. Given the rest of
Affineis fully annotated, adding them here would be consistent:- def compute_w_affine(cls, spatial_rank, mat, img_size, sp_size): + def compute_w_affine( + cls, + spatial_rank: int, + mat: NdarrayOrTensor, + img_size: Sequence[int], + sp_size: Sequence[int], + ) -> np.ndarray:🤖 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 2334 - 2358, Add precise type annotations to compute_w_affine and a concrete Returns entry in the docstring: annotate the signature as compute_w_affine(cls, spatial_rank: int, mat: Any, img_size: Sequence[int], sp_size: Sequence[int]) -> numpy.ndarray (or np.ndarray) and update the Returns section to say it returns a numpy.ndarray of shape (r+1, r+1) (where r = spatial_rank) representing the homogeneous affine matrix; keep references to the helper functions used (to_affine_nd, create_translate, convert_data_type) so reviewers can locate the code that guarantees the output shape and type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/transforms/test_affine.py`:
- Line 227: The list comprehension assigning expected_translation uses
zip(img_size, sp_size) without the strict flag; update that expression in
tests/transforms/test_affine.py (the expected_translation assignment) to call
zip(img_size, sp_size, strict=True) so the test enforces equal-length iterables
(keeping the rest of the list comprehension intact).
---
Nitpick comments:
In `@monai/transforms/spatial/array.py`:
- Around line 2334-2358: Add precise type annotations to compute_w_affine and a
concrete Returns entry in the docstring: annotate the signature as
compute_w_affine(cls, spatial_rank: int, mat: Any, img_size: Sequence[int],
sp_size: Sequence[int]) -> numpy.ndarray (or np.ndarray) and update the Returns
section to say it returns a numpy.ndarray of shape (r+1, r+1) (where r =
spatial_rank) representing the homogeneous affine matrix; keep references to the
helper functions used (to_affine_nd, create_translate, convert_data_type) so
reviewers can locate the code that guarantees the output shape and type.
ℹ️ Review info
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 (2)
monai/transforms/spatial/array.pytests/transforms/test_affine.py
engmohamedsalah
force-pushed
the
improve-affine-docs-7092
branch
from
70e1978 to
9c468d8
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/array.py (1)
2333-2358:⚠️ Potential issue | 🟡 MinorAdd type annotations and types in docstring.
The method signature has no type hints, and the Google-style docstring omits types in
Args/Returns. Per coding guidelines, both should be present.✏️ Proposed fix
- def compute_w_affine(cls, spatial_rank, mat, img_size, sp_size): + def compute_w_affine( + cls, + spatial_rank: int | float, + mat: NdarrayOrTensor, + img_size: Sequence[int], + sp_size: Sequence[int], + ) -> np.ndarray: """ ... Args: - spatial_rank: number of spatial dimensions (e.g., 2 for 2D, 3 for 3D). - mat: the base affine transformation matrix to be adjusted. - img_size: spatial dimensions of the input image. - sp_size: spatial dimensions of the output (transformed) image. + spatial_rank (int | float): number of spatial dimensions (e.g., 2 for 2D, 3 for 3D). + mat (NdarrayOrTensor): the base affine transformation matrix to be adjusted. + img_size (Sequence[int]): spatial dimensions of the input image. + sp_size (Sequence[int]): spatial dimensions of the output (transformed) image. Returns: - The adjusted affine matrix that can be applied to image coordinates. + np.ndarray: The adjusted affine matrix that can be applied to image coordinates. """As per coding guidelines, "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 the current code and only fix it if needed. In `@monai/transforms/spatial/array.py` around lines 2333 - 2358, Add Python type hints to the compute_w_affine signature and include types for each parameter and the return value in the Google-style docstring: annotate spatial_rank as int, mat as ArrayLike/np.ndarray (or Union of supported affine types), img_size and sp_size as Sequence[int] (or Tuple[int, ...]), and the return as np.ndarray; update the Args and Returns sections to include those types and short descriptions, and mention any exceptions raised (e.g., on invalid ranks) if applicable — edit the compute_w_affine method and its docstring accordingly.
🧹 Nitpick comments (1)
tests/transforms/test_affine.py (1)
202-241:TestComputeWAffine— LGTM on the basics; two improvements worth considering.The assertions are mathematically correct and the
strict=Falsefix is already applied. Two gaps:
test_different_sizeschecks only the translation column. Format=eye, the top-left2×2block is trivially identity, but the test doesn't assert it. A full-matrix comparison removes any hidden corner cases.No non-identity
mattest. The center-adjustment path is only meaningful whenmatisn't identity. A rotation or scale matrix as input would actually exerciseshift_1 @ mat @ shift_2.✏️ Suggested additions
def test_different_sizes(self): ... - assert_allclose(result[:2, 2], expected_translation, atol=1e-6) + expected = np.eye(3) + expected[:2, 2] = expected_translation + assert_allclose(result, expected, atol=1e-6) + def test_non_identity_mat_2d(self): + """Center-adjustment with a 90-degree rotation mat.""" + angle = np.pi / 2 + mat = np.array([[np.cos(angle), -np.sin(angle), 0], + [np.sin(angle), np.cos(angle), 0], + [0, 0, 1]], dtype=float) + img_size = (4, 4) + sp_size = (4, 4) + result = Affine.compute_w_affine(2, mat, img_size, sp_size) + # Same size + pure rotation: translation columns should be zero after center composition + assert_allclose(result[:2, :2], mat[:2, :2], atol=1e-6) + assert_allclose(result[:2, 2], np.zeros(2), atol=1e-6)🤖 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 202 - 241, Update TestComputeWAffine::test_different_sizes to assert the entire affine matrix (not just the translation column) when mat = np.eye(3); compute the expected full matrix by constructing the center shifts and verifying result equals shift_1 @ mat @ shift_2 (or equivalent full-matrix expression) so the top-left block and translation are tested together. Also add a new test (e.g., test_non_identity_mat) that passes a non-identity transform (scale or rotation matrix) into Affine.compute_w_affine and asserts the result equals the composed shift_1 @ mat @ shift_2 matrix to exercise the center-adjustment path and ensure compute_w_affine handles non-identity mat correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@monai/transforms/spatial/array.py`:
- Around line 2333-2358: Add Python type hints to the compute_w_affine signature
and include types for each parameter and the return value in the Google-style
docstring: annotate spatial_rank as int, mat as ArrayLike/np.ndarray (or Union
of supported affine types), img_size and sp_size as Sequence[int] (or Tuple[int,
...]), and the return as np.ndarray; update the Args and Returns sections to
include those types and short descriptions, and mention any exceptions raised
(e.g., on invalid ranks) if applicable — edit the compute_w_affine method and
its docstring accordingly.
---
Nitpick comments:
In `@tests/transforms/test_affine.py`:
- Around line 202-241: Update TestComputeWAffine::test_different_sizes to assert
the entire affine matrix (not just the translation column) when mat = np.eye(3);
compute the expected full matrix by constructing the center shifts and verifying
result equals shift_1 @ mat @ shift_2 (or equivalent full-matrix expression) so
the top-left block and translation are tested together. Also add a new test
(e.g., test_non_identity_mat) that passes a non-identity transform (scale or
rotation matrix) into Affine.compute_w_affine and asserts the result equals the
composed shift_1 @ mat @ shift_2 matrix to exercise the center-adjustment path
and ensure compute_w_affine handles non-identity mat correctly.
ℹ️ Review info
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 (2)
monai/transforms/spatial/array.pytests/transforms/test_affine.py
1 participant
Summary
This PR improves the documentation for the
Affinetransform and adds unit tests for thecompute_w_affinemethod.Fixes #7092
Changes
Documentation improvements (
monai/transforms/spatial/array.py)Affineclass documenting the center-origin coordinate system assumptionnormalizedparameter documentation with user-friendly explanationcompute_w_affineclassmethod (previously undocumented)Unit tests (
tests/transforms/test_affine.py)TestComputeWAffinetest class with focused tests:Verification
compute_w_affinetests passType of change