Skip to content

fix(data): avoid divide-by-zero in pydicom affine for single-slice volumes #8956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SID-6921 wants to merge 2 commits into
base: dev
Choose a base branch
from
+36 −1
Open

fix(data): avoid divide-by-zero in pydicom affine for single-slice volumes #8956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a51549
fix(data): avoid divide-by-zero in pydicom affine for single-slice vo…
SID-6921 Jun 27, 2026
6b5f388
test(data): skip pydicom affine tests when Pydicom is unavailable
SID-6921 Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion monai/data/image_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ def _get_affine(self, metadata: dict, lps_to_ras: bool = True):
affine[2, 3] = sz

# 3d
if "lastImagePositionPatient" in metadata:
if "lastImagePositionPatient" in metadata and metadata[MetaKeys.SPATIAL_SHAPE][-1] > 1:
t1n, t2n, t3n = metadata["lastImagePositionPatient"]
n = metadata[MetaKeys.SPATIAL_SHAPE][-1]
k1, k2, k3 = (t1n - sx) / (n - 1), (t2n - sy) / (n - 1), (t3n - sz) / (n - 1)
Expand Down
35 changes: 35 additions & 0 deletions tests/data/test_init_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

from monai.data import ITKReader, NibabelReader, NrrdReader, NumpyReader, PILReader, PydicomReader
from monai.transforms import LoadImage, LoadImaged
from monai.utils import MetaKeys
from tests.test_utils import SkipIfNoModule


Expand Down Expand Up @@ -100,6 +101,40 @@ def test_nibabel_reader_avoids_eager_c_order_copy(self):
# (F-order) layout from nibabel should be preserved here.
self.assertFalse(data.flags.c_contiguous)

@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)

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)

@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)

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)
Comment on lines +122 to +136

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.


Comment on lines +105 to +137

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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


if __name__ == "__main__":
unittest.main()
Loading