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 2 commits into
Conversation
|
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:
📝 WalkthroughWalkthrough
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_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.
There was a problem hiding this comment.
@SID-6921 please have a look at this comment, I think the change makes sense. It's possible the tests weren't being run if the module name was "Pydicom" instead of "pydicom".
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd test_utils.py tests . -a
ast-grep outline tests/test_utils.py --view expanded || true
rg -n -C3 'class SkipIfNoModule|def SkipIfNoModule|SkipIfNoModule\(' tests/test_utils.py tests -g '!**/.git/**'
sed -n '1,220p' tests/test_utils.pyLength of output: 42206
@ericspod Good catch.
I checked tests/test_utils.py: SkipIfNoModule calls optional_import(self.module_name) directly, so the module string matters here. In tests/data/test_init_reader.py these decorators use "Pydicom", while other tests use "pydicom".
So yes:
- changing the decorators to
@SkipIfNoModule("pydicom")looks correct, and - strengthening the multi-slice fixture still makes sense so the branch is actually proven.
🧬 Code Graph Analysis Results
tests/data/test_init_reader.py (around the Pydicom skip decorators)
Lines 49-76 (optional dependency gating for reader construction):
`@SkipIfNoModule`("itk")
`@SkipIfNoModule`("nibabel")
`@SkipIfNoModule`("PIL")
`@SkipIfNoModule`("nrrd")
`@SkipIfNoModule`("Pydicom")
def test_readers(self):
inst = ITKReader()
self.assertIsInstance(inst, ITKReader)
...
inst = PydicomReader()
self.assertIsInstance(inst, PydicomReader)
...
inst = NrrdReader()
self.assertIsInstance(inst, NrrdReader)Lines 105-121 (optional dependency gating for _get_affine, single-slice case):
`@SkipIfNoModule`("Pydicom")
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)Lines 122-138 (optional dependency gating for _get_affine, multi-slice case):
`@SkipIfNoModule`("Pydicom")
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)tests/test_utils.py (the skip decorator used by the tests)
Lines 262-271:
class SkipIfNoModule:
"""Decorator to be used if test should be skipped
when optional module is not present."""
def __init__(self, module_name):
self.module_name = module_name
self.module_missing = not optional_import(self.module_name)[1]
def __call__(self, obj):
return unittest.skipIf(self.module_missing, f"optional module not present: {self.module_name}")(obj)monai/data/image_reader.py (method under test: _get_affine behavior)
PydicomReader._get_affine — Lines 729-771:
def _get_affine(self, metadata: dict, lps_to_ras: bool = True):
"""
Get or construct the affine matrix of the image, it can be used to correct
spacing, orientation or execute spatial transforms.
Args:
metadata: metadata with dict type.
lps_to_ras: whether to convert the affine matrix from "LPS" to "RAS". Defaults to True.
"""
affine: np.ndarray = np.eye(4)
if not ("00200037" in metadata and "00200032" in metadata):
return affine
# "00200037" is the tag of `ImageOrientationPatient`
rx, ry, rz, cx, cy, cz = metadata["00200037"]["Value"]
# "00200032" is the tag of `ImagePositionPatient`
sx, sy, sz = metadata["00200032"]["Value"]
# "00280030" is the tag of `PixelSpacing`
spacing = metadata["00280030"]["Value"] if "00280030" in metadata else (1.0, 1.0)
dr, dc = metadata.get("spacing", spacing)[:2]
affine[0, 0] = cx * dr
affine[0, 1] = rx * dc
affine[0, 3] = sx
affine[1, 0] = cy * dr
affine[1, 1] = ry * dc
affine[1, 3] = sy
affine[2, 0] = cz * dr
affine[2, 1] = rz * dc
affine[2, 2] = 1.0
affine[2, 3] = sz
# 3d
if "lastImagePositionPatient" in metadata:
t1n, t2n, t3n = metadata["lastImagePositionPatient"]
n = metadata[MetaKeys.SPATIAL_SHAPE][-1]
if n > 1:
affine[0, 2] = (t1n - sx) / (n - 1)
affine[1, 2] = (t2n - sy) / (n - 1)
affine[2, 2] = (t3n - sz) / (n - 1)
if lps_to_ras:
affine = orientation_ras_lps(affine)
return affine📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
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. |
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). |
| # (F-order) layout from nibabel should be preserved here. | ||
| self.assertFalse(data.flags.c_contiguous) | ||
|
|
||
| @SkipIfNoModule("Pydicom") |
There was a problem hiding this comment.
| @SkipIfNoModule("Pydicom") | |
| @SkipIfNoModule("pydicom") |
Module names are case sensitive, same in the next test.
|
Thanks @SID-6921 I had minor comments about the module name and the Coderabbit comment is worth looking at. |
|
Thanks for the review comments. I applied the module-name fix and force-pushed: all SkipIfNoModule uses for pydicom in this test file are now lowercase (pydicom), including the two new affine regression tests. Local pytest and pre-commit passed after this change. |
…etadata Signed-off-by: SID <nandasiddhardha@gmail.com>
Signed-off-by: SID <nandasiddhardha@gmail.com>
SID-6921
force-pushed
the
fix/pydicom-affine-single-slice-zero-div
branch
from
fc56a23 to
42517f5
Compare
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