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
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4ed8a6
test(data): add affine regression coverage for single-slice pydicom m…
SID-6921 Jun 30, 2026
42517f5
test(data): use lowercase pydicom module name in SkipIfNoModule
SID-6921 Jun 30, 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
37 changes: 36 additions & 1 deletion 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 @@ -48,7 +49,7 @@ def test_load_image_to_gpu(self):
@SkipIfNoModule("nibabel")
@SkipIfNoModule("PIL")
@SkipIfNoModule("nrrd")
@SkipIfNoModule("Pydicom")
@SkipIfNoModule("pydicom")
def test_readers(self):
inst = ITKReader()
self.assertIsInstance(inst, ITKReader)
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 •
edited
Loading

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.

@ericspod ericspod Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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".

@coderabbitai coderabbitai Bot Jun 30, 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.

🧩 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.py

Length 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


Comment thread
ericspod marked this conversation as resolved.

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