Fix compute_shape_offset non-tuple indexing for PyTorch >=2.9#8776
Open
Harrisaint wants to merge 6 commits intoProject-MONAI:devfrom
Open
Fix compute_shape_offset non-tuple indexing for PyTorch >=2.9#8776Harrisaint wants to merge 6 commits intoProject-MONAI:devfrom
Harrisaint wants to merge 6 commits intoProject-MONAI:devfrom
Conversation
Signed-off-by: Harrisaint <hgmartin1116@gmail.com>
Contributor
|
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:
📝 WalkthroughWalkthroughConvert Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
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/data/utils.py`:
- Line 884: Add a regression unit test for monai.data.utils.compute_shape_offset
that ensures the tuple cast fix handles torch types: call compute_shape_offset
with spatial_shape provided as a torch.Tensor and as a torch.Size (and
optionally a subclass) and assert it returns the same numeric result as when
called with a plain tuple/list and does not raise; target the function
compute_shape_offset and its spatial_shape parameter so future changes to the
line that constructs shape = np.array(tuple(spatial_shape), copy=True,
dtype=float) will be exercised by CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cbdf7d7-5417-4a07-a137-7af40388f077
📒 Files selected for processing (1)
monai/data/utils.py
Signed-off-by: Harrisaint <hgmartin1116@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/data/utils/test_compute_shape_offset.py (3)
14-14: Unused variableoffset.Rename to
_to signal intentional discard and silence the linter.Fix
- shape, offset = compute_shape_offset(spatial_shape, in_affine, out_affine) + shape, _ = compute_shape_offset(spatial_shape, in_affine, out_affine)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/utils/test_compute_shape_offset.py` at line 14, The test calls compute_shape_offset(spatial_shape, in_affine, out_affine) but assigns the unused second return value to offset; rename that variable to _ to indicate intentional discard and silence linter warnings (change the tuple assignment in tests/data/utils/test_compute_shape_offset.py from "shape, offset = compute_shape_offset(...)" to "shape, _ = compute_shape_offset(...)" so only shape is used and the unused return value is ignored).
6-17: Strengthen assertions and add docstrings.The test only checks
len(shape) == 3, which merely confirms the function didn't crash. With identity affines, the output shape should equal the input[10, 10, 10]. Verifying actual values catches regressions in the arithmetic. Also, per coding guidelines, docstrings are required.Suggested improvements
class TestComputeShapeOffsetRegression(unittest.TestCase): + """Regression tests for compute_shape_offset handling various input types.""" + def test_pytorch_size_input(self): - # 1 Create a PyTorch Size object (which triggered the original bug) + """Test that torch.Size inputs are correctly converted and processed. + + Verifies fix for issue `#8775` where torch.Size caused errors on PyTorch >=2.9. + """ spatial_shape = torch.Size([10, 10, 10]) in_affine = np.eye(4) out_affine = np.eye(4) - # 2 Feed it into the function - shape, offset = compute_shape_offset(spatial_shape, in_affine, out_affine) + shape, _ = compute_shape_offset(spatial_shape, in_affine, out_affine) - # 3 Prove it successfully processed the shape by checking its length self.assertEqual(len(shape), 3) + np.testing.assert_array_equal(shape, [10, 10, 10])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/utils/test_compute_shape_offset.py` around lines 6 - 17, Update the test TestComputeShapeOffsetRegression.test_pytorch_size_input to include a docstring describing the regression scenario and strengthen assertions: after calling compute_shape_offset(spatial_shape, in_affine, out_affine) assert that the returned shape equals the expected [10, 10, 10] (or tuple equivalent) rather than only checking its length, and also assert that offset is of the expected type/shape if applicable; reference the compute_shape_offset function and the test_pytorch_size_input method when making these changes.
1-4: Reorder imports per PEP8.Standard library imports (
unittest) should precede third-party imports (numpy,torch).Suggested fix
-import torch -import unittest -import numpy as np +import unittest + +import numpy as np +import torch from monai.data.utils import compute_shape_offset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/utils/test_compute_shape_offset.py` around lines 1 - 4, Reorder the imports in the test file so standard library imports come first per PEP8: move the unittest import above third-party imports (numpy, torch, and monai). Keep the import of compute_shape_offset from monai.data.utils and the existing torch/numpy imports intact but follow unittest with a blank line before the third-party group to match PEP8 grouping.
🤖 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/data/utils/test_compute_shape_offset.py`:
- Line 14: The test calls compute_shape_offset(spatial_shape, in_affine,
out_affine) but assigns the unused second return value to offset; rename that
variable to _ to indicate intentional discard and silence linter warnings
(change the tuple assignment in tests/data/utils/test_compute_shape_offset.py
from "shape, offset = compute_shape_offset(...)" to "shape, _ =
compute_shape_offset(...)" so only shape is used and the unused return value is
ignored).
- Around line 6-17: Update the test
TestComputeShapeOffsetRegression.test_pytorch_size_input to include a docstring
describing the regression scenario and strengthen assertions: after calling
compute_shape_offset(spatial_shape, in_affine, out_affine) assert that the
returned shape equals the expected [10, 10, 10] (or tuple equivalent) rather
than only checking its length, and also assert that offset is of the expected
type/shape if applicable; reference the compute_shape_offset function and the
test_pytorch_size_input method when making these changes.
- Around line 1-4: Reorder the imports in the test file so standard library
imports come first per PEP8: move the unittest import above third-party imports
(numpy, torch, and monai). Keep the import of compute_shape_offset from
monai.data.utils and the existing torch/numpy imports intact but follow unittest
with a blank line before the third-party group to match PEP8 grouping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c98fb3dd-915c-4c9a-b812-9024db1165c6
📒 Files selected for processing (1)
tests/data/utils/test_compute_shape_offset.py
Signed-off-by: Harrisaint <hgmartin1116@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/data/utils/test_compute_shape_offset.py (1)
17-18: Add Google-style docstrings to new definitions.Please add docstrings for the test class and test method per repository rules.
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 `@tests/data/utils/test_compute_shape_offset.py` around lines 17 - 18, Add Google-style docstrings to the TestComputeShapeOffsetRegression class and its test_pytorch_size_input method: for the class provide a brief summary of the test purpose and any class-level attributes, and for test_pytorch_size_input add a docstring with a one-line summary, an Args section (if the test uses any parameters/fixtures), a Returns section (state that it returns None), and a Raises section listing expected assertion errors; update the docstrings to follow Google-style formatting so tools and linters recognize them.
🤖 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/data/utils/test_compute_shape_offset.py`:
- Line 25: The test binds an unused second return value from
compute_shape_offset as offset, triggering RUF059; change the unpack to capture
the unused value as _offset or _ (e.g., replace "shape, offset =
compute_shape_offset(...)" with "shape, _offset = compute_shape_offset(...)" or
"shape, _ = compute_shape_offset(...)") and ensure no subsequent code relies on
the old name so the linter warning is resolved.
- Around line 27-28: Replace the weak len(shape) assertion with explicit checks
of the computed shape (and offset when appropriate): locate the test in
tests/data/utils/test_compute_shape_offset.py that currently does
self.assertEqual(len(shape), 3) and change it to assert that shape equals the
expected tuple of dimensions (e.g., (x, y, z) for the input fixture) and, for
cases using an identity affine or where an offset is deterministic, assert
offset equals the expected coordinate tuple (e.g., (0,0,0) or the known offset).
Ensure you reference the test variables shape and offset (and the
compute_shape_offset helper if present) so the test fails on wrong values rather
than only wrong rank.
---
Nitpick comments:
In `@tests/data/utils/test_compute_shape_offset.py`:
- Around line 17-18: Add Google-style docstrings to the
TestComputeShapeOffsetRegression class and its test_pytorch_size_input method:
for the class provide a brief summary of the test purpose and any class-level
attributes, and for test_pytorch_size_input add a docstring with a one-line
summary, an Args section (if the test uses any parameters/fixtures), a Returns
section (state that it returns None), and a Raises section listing expected
assertion errors; update the docstrings to follow Google-style formatting so
tools and linters recognize them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2981a967-6ad7-48cb-af71-690b2ab86b3b
📒 Files selected for processing (1)
tests/data/utils/test_compute_shape_offset.py
Signed-off-by: Harrisaint <hgmartin1116@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/data/utils/test_compute_shape_offset.py (1)
20-21: Missing docstrings.Per coding guidelines, test classes and methods should have docstrings describing their purpose. Add a brief docstring explaining this is a regression test for PyTorch ≥2.9 torch.Size handling.
Proposed fix
class TestComputeShapeOffsetRegression(unittest.TestCase): + """Regression tests for compute_shape_offset input handling.""" + def test_pytorch_size_input(self): + """Verify torch.Size inputs are converted correctly (PyTorch ≥2.9 fix).""" # 1 Create a PyTorch Size object (which triggered the original bug)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/utils/test_compute_shape_offset.py` around lines 20 - 21, Add missing docstrings to the test class TestComputeShapeOffsetRegression and its method test_pytorch_size_input: add a brief class-level docstring stating this is a regression test for torch.Size handling in PyTorch ≥2.9, and add a method-level docstring describing that test_pytorch_size_input verifies compute_shape_offset correctly accepts/handles torch.Size inputs introduced/changed in PyTorch 2.9. Ensure the docstrings are concise and follow the project's style for test descriptions.
🤖 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/data/utils/test_compute_shape_offset.py`:
- Around line 20-21: Add missing docstrings to the test class
TestComputeShapeOffsetRegression and its method test_pytorch_size_input: add a
brief class-level docstring stating this is a regression test for torch.Size
handling in PyTorch ≥2.9, and add a method-level docstring describing that
test_pytorch_size_input verifies compute_shape_offset correctly accepts/handles
torch.Size inputs introduced/changed in PyTorch 2.9. Ensure the docstrings are
concise and follow the project's style for test descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f85e913a-a35f-4a71-966e-30ee68e064fa
📒 Files selected for processing (1)
tests/data/utils/test_compute_shape_offset.py
Signed-off-by: Harrisaint <hgmartin1116@gmail.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/data/utils/test_compute_shape_offset.py (1)
30-33:⚠️ Potential issue | 🟡 MinorStrengthen this regression check to validate values, not just rank.
Line 33 only checks dimensionality. For this identity-affine case, assert exact
shapeandoffsetvalues so regressions in result computation are caught; this also addresses the unusedoffsetbound at Line 30.Proposed patch
- shape, offset = compute_shape_offset(spatial_shape, in_affine, out_affine) + shape, offset = compute_shape_offset(spatial_shape, in_affine, out_affine) @@ - # 3. Prove it successfully processed the shape by checking its length - self.assertEqual(len(shape), 3) + # 3. Verify concrete outputs for deterministic identity-affine case + self.assertTupleEqual(tuple(shape), (10, 10, 10)) + self.assertTrue(np.allclose(offset, (0.0, 0.0, 0.0)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/utils/test_compute_shape_offset.py` around lines 30 - 33, The test currently only checks len(shape) after calling compute_shape_offset(spatial_shape, in_affine, out_affine); instead assert exact values for both shape and offset for the identity-affine case: verify that shape equals the input spatial_shape and that offset is an all-zero sequence matching the dimensionality (e.g. (0,0,0) or [0]*len(spatial_shape)). Update the assertions to use the variables shape and offset returned from compute_shape_offset and compare them to spatial_shape and a zero-filled tuple/list to catch regressions in compute_shape_offset.
🧹 Nitpick comments (1)
tests/data/utils/test_compute_shape_offset.py (1)
22-23: Add Google-style docstrings to new definitions.The new class and test method are missing docstrings.
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."
Proposed patch
class TestComputeShapeOffsetRegression(unittest.TestCase): + """Regression tests for `compute_shape_offset` input-shape handling.""" + def test_pytorch_size_input(self): + """Validate `torch.Size` input produces expected shape and offset. + + Returns: + None. + + Raises: + AssertionError: If computed shape/offset are not as expected. + """ # 1. Create a PyTorch Size object (which triggered the original bug)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/utils/test_compute_shape_offset.py` around lines 22 - 23, Add Google-style docstrings to the new test class TestComputeShapeOffsetRegression and its method test_pytorch_size_input: for the class include a short description of what regression or behavior the tests cover and any shared attributes; for the test method include a one-line summary, Args section documenting "self", Returns section noting None, and Raises section if the test expects exceptions (or "None" otherwise). Ensure both docstrings follow Google-style sections (Args, Returns, Raises) and are placed immediately under the class and method definitions respectively.
🤖 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/data/utils/test_compute_shape_offset.py`:
- Around line 30-33: The test currently only checks len(shape) after calling
compute_shape_offset(spatial_shape, in_affine, out_affine); instead assert exact
values for both shape and offset for the identity-affine case: verify that shape
equals the input spatial_shape and that offset is an all-zero sequence matching
the dimensionality (e.g. (0,0,0) or [0]*len(spatial_shape)). Update the
assertions to use the variables shape and offset returned from
compute_shape_offset and compare them to spatial_shape and a zero-filled
tuple/list to catch regressions in compute_shape_offset.
---
Nitpick comments:
In `@tests/data/utils/test_compute_shape_offset.py`:
- Around line 22-23: Add Google-style docstrings to the new test class
TestComputeShapeOffsetRegression and its method test_pytorch_size_input: for the
class include a short description of what regression or behavior the tests cover
and any shared attributes; for the test method include a one-line summary, Args
section documenting "self", Returns section noting None, and Raises section if
the test expects exceptions (or "None" otherwise). Ensure both docstrings follow
Google-style sections (Args, Returns, Raises) and are placed immediately under
the class and method definitions respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f590910-d66f-475b-b02e-473ed89dbe37
📒 Files selected for processing (1)
tests/data/utils/test_compute_shape_offset.py
Signed-off-by: Harrisaint <hgmartin1116@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8775 .
Description
This PR resolves issue #8775 by casting spatial_shape to a tuple inside compute_shape_offset to prevent breaking changes in PyTorch >= 2.9.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.