fix(data): avoid divide-by-zero in pydicom affine for single-slice volumes#8956
fix(data): avoid divide-by-zero in pydicom affine for single-slice volumes#8956SID-6921 wants to merge 1 commit into
Conversation
|
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo unit tests are added to Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/data/image_reader.py (1)
729-771: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new
_get_affinefallback.This definition changed, but its docstring still omits a
Returnssection and does not say thatlastImagePositionPatientis ignored unlessMetaKeys.SPATIAL_SHAPE[-1] > 1. Please document that single-slice inputs keep the default third column. 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/data/image_reader.py` around lines 729 - 771, The _get_affine docstring is missing the new fallback behavior and return documentation. Update the Google-style docstring for _get_affine to include a Returns section for the affine matrix, and note that lastImagePositionPatient is only used when MetaKeys.SPATIAL_SHAPE[-1] > 1; for single-slice inputs the default third column remains unchanged.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.
Inline comments:
In `@tests/data/test_init_reader.py`:
- Around line 104-135: Add Google-style docstrings to the two new test methods
in PydicomReader’s test class:
test_pydicom_reader_get_affine_single_slice_with_last_position and
test_pydicom_reader_get_affine_multi_slice_uses_last_position. Briefly describe
the purpose of each test, the key metadata variables they set up, and the
expected affine assertions in the Args/Returns/Raises sections as appropriate,
matching the project’s docstring requirements for test definitions.
- Around line 120-134: The PydicomReader._get_affine multi-slice test is too
weak because the current lastImagePositionPatient and MetaKeys.SPATIAL_SHAPE
values produce the same affine result as the default branch, so it does not
prove the special path executed. Update
test_pydicom_reader_get_affine_multi_slice_uses_last_position to use a
lastImagePositionPatient value and spatial shape that yield a non-default k3
term, then assert that affine[2, 2] (or the full third column) reflects the
branch-specific result so the test fails if _get_affine skips the
lastImagePositionPatient logic.
---
Outside diff comments:
In `@monai/data/image_reader.py`:
- Around line 729-771: The _get_affine docstring is missing the new fallback
behavior and return documentation. Update the Google-style docstring for
_get_affine to include a Returns section for the affine matrix, and note that
lastImagePositionPatient is only used when MetaKeys.SPATIAL_SHAPE[-1] > 1; for
single-slice inputs the default third column remains unchanged.
🪄 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: 2ddf1ea8-6e46-466f-b496-8d91deca2125
📒 Files selected for processing (2)
monai/data/image_reader.pytests/data/test_init_reader.py
| def test_pydicom_reader_get_affine_single_slice_with_last_position(self): | ||
| reader = PydicomReader() | ||
| metadata = { | ||
| "00200037": {"Value": [1.0, 0.0, 0.0, 0.0, 1.0, 0.0]}, | ||
| "00200032": {"Value": [10.0, 20.0, 30.0]}, | ||
| "00280030": {"Value": [0.5, 0.25]}, | ||
| "lastImagePositionPatient": np.array([10.0, 20.0, 30.0]), | ||
| MetaKeys.SPATIAL_SHAPE: np.array([64, 64, 1]), | ||
| } | ||
|
|
||
| affine = reader._get_affine(metadata, lps_to_ras=False) | ||
|
|
||
| np.testing.assert_allclose(affine[0, 2], 0.0) | ||
| np.testing.assert_allclose(affine[1, 2], 0.0) | ||
| np.testing.assert_allclose(affine[2, 2], 1.0) | ||
|
|
||
| def test_pydicom_reader_get_affine_multi_slice_uses_last_position(self): | ||
| reader = PydicomReader() | ||
| metadata = { | ||
| "00200037": {"Value": [1.0, 0.0, 0.0, 0.0, 1.0, 0.0]}, | ||
| "00200032": {"Value": [0.0, 0.0, 0.0]}, | ||
| "00280030": {"Value": [1.0, 1.0]}, | ||
| "lastImagePositionPatient": np.array([0.0, 0.0, 4.0]), | ||
| MetaKeys.SPATIAL_SHAPE: np.array([8, 8, 5]), | ||
| } | ||
|
|
||
| affine = reader._get_affine(metadata, lps_to_ras=False) | ||
|
|
||
| np.testing.assert_allclose(affine[0, 2], 0.0) | ||
| np.testing.assert_allclose(affine[1, 2], 0.0) | ||
| np.testing.assert_allclose(affine[2, 2], 1.0) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add docstrings to the new tests.
Both new test defs are missing Google-style docstrings. 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/data/test_init_reader.py` around lines 104 - 135, Add Google-style
docstrings to the two new test methods in PydicomReader’s test class:
test_pydicom_reader_get_affine_single_slice_with_last_position and
test_pydicom_reader_get_affine_multi_slice_uses_last_position. Briefly describe
the purpose of each test, the key metadata variables they set up, and the
expected affine assertions in the Args/Returns/Raises sections as appropriate,
matching the project’s docstring requirements for test definitions.
Source: Path instructions
| def test_pydicom_reader_get_affine_multi_slice_uses_last_position(self): | ||
| reader = PydicomReader() | ||
| metadata = { | ||
| "00200037": {"Value": [1.0, 0.0, 0.0, 0.0, 1.0, 0.0]}, | ||
| "00200032": {"Value": [0.0, 0.0, 0.0]}, | ||
| "00280030": {"Value": [1.0, 1.0]}, | ||
| "lastImagePositionPatient": np.array([0.0, 0.0, 4.0]), | ||
| MetaKeys.SPATIAL_SHAPE: np.array([8, 8, 5]), | ||
| } | ||
|
|
||
| affine = reader._get_affine(metadata, lps_to_ras=False) | ||
|
|
||
| np.testing.assert_allclose(affine[0, 2], 0.0) | ||
| np.testing.assert_allclose(affine[1, 2], 0.0) | ||
| np.testing.assert_allclose(affine[2, 2], 1.0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the multi-slice fixture prove the branch ran.
With lastImagePositionPatient = [0, 0, 4] and MetaKeys.SPATIAL_SHAPE[-1] = 5, _get_affine() computes (k1, k2, k3) == (0, 0, 1), which is exactly the same as the pre-branch defaults. This test still passes if the lastImagePositionPatient path is skipped entirely.
Proposed fix
def test_pydicom_reader_get_affine_multi_slice_uses_last_position(self):
reader = PydicomReader()
metadata = {
"00200037": {"Value": [1.0, 0.0, 0.0, 0.0, 1.0, 0.0]},
"00200032": {"Value": [0.0, 0.0, 0.0]},
"00280030": {"Value": [1.0, 1.0]},
- "lastImagePositionPatient": np.array([0.0, 0.0, 4.0]),
+ "lastImagePositionPatient": np.array([2.0, 4.0, 8.0]),
MetaKeys.SPATIAL_SHAPE: np.array([8, 8, 5]),
}
affine = reader._get_affine(metadata, lps_to_ras=False)
- np.testing.assert_allclose(affine[0, 2], 0.0)
- np.testing.assert_allclose(affine[1, 2], 0.0)
- np.testing.assert_allclose(affine[2, 2], 1.0)
+ np.testing.assert_allclose(affine[0, 2], 0.5)
+ np.testing.assert_allclose(affine[1, 2], 1.0)
+ np.testing.assert_allclose(affine[2, 2], 2.0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_pydicom_reader_get_affine_multi_slice_uses_last_position(self): | |
| reader = PydicomReader() | |
| metadata = { | |
| "00200037": {"Value": [1.0, 0.0, 0.0, 0.0, 1.0, 0.0]}, | |
| "00200032": {"Value": [0.0, 0.0, 0.0]}, | |
| "00280030": {"Value": [1.0, 1.0]}, | |
| "lastImagePositionPatient": np.array([0.0, 0.0, 4.0]), | |
| MetaKeys.SPATIAL_SHAPE: np.array([8, 8, 5]), | |
| } | |
| affine = reader._get_affine(metadata, lps_to_ras=False) | |
| np.testing.assert_allclose(affine[0, 2], 0.0) | |
| np.testing.assert_allclose(affine[1, 2], 0.0) | |
| np.testing.assert_allclose(affine[2, 2], 1.0) | |
| def test_pydicom_reader_get_affine_multi_slice_uses_last_position(self): | |
| reader = PydicomReader() | |
| metadata = { | |
| "00200037": {"Value": [1.0, 0.0, 0.0, 0.0, 1.0, 0.0]}, | |
| "00200032": {"Value": [0.0, 0.0, 0.0]}, | |
| "00280030": {"Value": [1.0, 1.0]}, | |
| "lastImagePositionPatient": np.array([2.0, 4.0, 8.0]), | |
| MetaKeys.SPATIAL_SHAPE: np.array([8, 8, 5]), | |
| } | |
| affine = reader._get_affine(metadata, lps_to_ras=False) | |
| np.testing.assert_allclose(affine[0, 2], 0.5) | |
| np.testing.assert_allclose(affine[1, 2], 1.0) | |
| np.testing.assert_allclose(affine[2, 2], 2.0) |
🤖 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/data/test_init_reader.py` around lines 120 - 134, The
PydicomReader._get_affine multi-slice test is too weak because the current
lastImagePositionPatient and MetaKeys.SPATIAL_SHAPE values produce the same
affine result as the default branch, so it does not prove the special path
executed. Update test_pydicom_reader_get_affine_multi_slice_uses_last_position
to use a lastImagePositionPatient value and spatial shape that yield a
non-default k3 term, then assert that affine[2, 2] (or the full third column)
reflects the branch-specific result so the test fails if _get_affine skips the
lastImagePositionPatient logic.
|
|
||
| # 3d | ||
| if "lastImagePositionPatient" in metadata: | ||
| if "lastImagePositionPatient" in metadata and metadata[MetaKeys.SPATIAL_SHAPE][-1] > 1: |
There was a problem hiding this comment.
Hi @SID-6921 I'm afraid we just merged another PR that addressed this issue. Could you please look at the current state of the repo and see if that satisfies the issue? If so we can drop the change in this file but keep any of your tests that add value. Thanks!
|
Thanks for the heads-up. I synced with current dev and removed the duplicate change in monai/data/image_reader.py from this PR. I kept the regression tests in ests/data/test_init_reader.py (single-slice and multi-slice affine metadata coverage), which still add value. Local validation passes for the updated PR scope. |
…etadata Signed-off-by: SID <nandasiddhardha@gmail.com>
SID-6921
force-pushed
the
fix/pydicom-affine-single-slice-zero-div
branch
from
a8d8ca6 to
6140c73
Compare
|
Update done: I rebased the PR branch onto current dev and force-pushed it so the duplicate image_reader.py fix is no longer part of this PR. The PR now keeps only the added regression coverage in ests/data/test_init_reader.py (single-slice and multi-slice affine metadata cases, guarded by optional Pydicom availability). |
2 participants
Summary:
Why:
For single-slice 3D DICOM segmentation metadata, n == 1 caused division by zero in affine z-step computation.
Validation:
Closes #8925