Fix #8775: handle torch.Tensor input in compute_shape_offset#8812
Fix #8775: handle torch.Tensor input in compute_shape_offset#8812williams145 wants to merge 2 commits intoProject-MONAI:devfrom
Conversation
…fset np.array(spatial_shape) fails with PyTorch >= 2.9 when spatial_shape is a torch.Tensor, because PyTorch removed the non-tuple sequence indexing protocol from tensors in that release. Wrapping with tuple() first routes through __iter__, yielding scalar 0-d tensors that NumPy can consume via the scalar protocol. This makes the function compatible with torch.Tensor, np.ndarray, and plain sequence inputs alike. The caller in monai/transforms/spatial/functional.py (line 115) already passes a torch.Tensor, so this is a real-world breakage on PyTorch 2.9. Signed-off-by: UGBOMEH OGOCHUKWU WILLIAMS <williamsugbomeh@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces construction of Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/data/utils/test_compute_shape_offset.py (1)
24-45: Add docstrings for new test definitions.
setUpand the newtest_*methods currently lack 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."
🤖 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 24 - 45, Add Google-style docstrings to the test methods setUp, test_numpy_array_input, test_list_input, test_torch_tensor_input, and test_identity_affines_preserve_shape describing purpose, arguments (if any), return (None), and any exceptions; for example, document that setUp initializes self.affine, that each test calls compute_shape_offset with different shape input types and asserts expected output length or values, and that no exceptions are expected. Keep docstrings brief and placed immediately under each function definition.
🤖 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`:
- Around line 29-44: Tests in test_compute_shape_offset.py unpack
compute_shape_offset into (out_shape, offset) but never use offset, triggering
an unused variable lint warning; update the test cases (those calling
compute_shape_offset in test_list_input, test_torch_tensor_input,
test_identity_affines_preserve_shape and the earlier scalar input) to either
unpack as (out_shape, _) or explicitly assert offset when relevant so the unused
`offset` is removed — locate the calls to compute_shape_offset and change the
second variable name to `_` (or add a meaningful assert on `offset` where the
test intends to validate it).
---
Nitpick comments:
In `@tests/data/utils/test_compute_shape_offset.py`:
- Around line 24-45: Add Google-style docstrings to the test methods setUp,
test_numpy_array_input, test_list_input, test_torch_tensor_input, and
test_identity_affines_preserve_shape describing purpose, arguments (if any),
return (None), and any exceptions; for example, document that setUp initializes
self.affine, that each test calls compute_shape_offset with different shape
input types and asserts expected output length or values, and that no exceptions
are expected. Keep docstrings brief and placed immediately under each function
definition.
🪄 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: c0c7abd9-cf23-46e3-9fa1-2b7c49d4ed74
📒 Files selected for processing (2)
monai/data/utils.pytests/data/utils/test_compute_shape_offset.py
… unused variable - Add Google-style docstrings to class and all test methods to satisfy docstring coverage CI check (was 16.67%, needed 80%) - Replace unused `offset` variable with `_` in four test cases (RUF059) Signed-off-by: UGBOMEH OGOCHUKWU WILLIAMS <williamsugbomeh@gmail.com>
|
The quick-py3 (macOS-latest) failure is a pre-existing infrastructure issue, pytype 2024.4.11 fails to install on the macOS runner due to a missing pybind11 dependency, which is unrelated to the changes in this PR. The same failure appears on other open PRs against dev for the same reason. |
|
Hi @williams145 thanks for this fix, this was already in the works in #8776 however. What I'd suggest is to merge that one first then merge in your extended test set which I think are good to add. We have to merge #8814 before this can be merged however which should address the pybind11 issue. |
PR #8814 has been merged. The pybind11 blocker should now be resolved, this should mean that CI can be re-triggered when convenient. |
atharvajoshi01
left a comment
There was a problem hiding this comment.
Converting to tuple before passing to np.array is a good fix for PyTorch >= 2.9 which deprecated non-tuple sequence indexing. Test coverage looks solid — covers numpy, list, tuple, and torch.Tensor inputs.
Description
compute_shape_offsetinmonai/data/utils.pypassesspatial_shapedirectly tonp.array(). Whenspatial_shapeis atorch.Tensor, this relies on the non-tuple sequence indexing protocol, which PyTorch removed in version 2.9. The call raises a hard error on PyTorch ≥ 2.9.The fix is to wrap
spatial_shapeintuple()before passing it tonp.array(). This routes through__iter__, which is stable across all PyTorch versions, and produces 0-d scalar tensors that NumPy consumes correctly.Root cause
The direct caller in
monai/transforms/spatial/functional.py(line 115) constructs anin_spatial_sizeas atorch.Tensorand passes it straight tocompute_shape_offset. This path has been broken since PyTorch 2.9.Changes
monai/data/utils.py— one-character change:np.array(spatial_shape, ...)→np.array(tuple(spatial_shape), ...)tests/data/utils/test_compute_shape_offset.py: new unit tests coveringtorch.Tensor,np.ndarray, and plain list inputsFixes #8775