From 2862c9ad3ce9b1f76d21fc61fe737284d67902d4 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 16 Sep 2024 12:59:13 -0400 Subject: [PATCH 1/5] RF: Gather/use list of all dicom files to avoid calling wrapper_from_file at module level I think it is safer to avoid 3rd party library calls at the module import time since if they are buggy it would be too loud of kaboom. Also it adds CPU time at import time even if not used later on. So replaced with defining a list of all DICOMs and then just using that function directly from filename. This also would run it against more DICOMs as this time we would glob recursively. --- heudiconv/tests/test_dicoms.py | 20 ++++++-------------- heudiconv/tests/utils.py | 9 +++++++++ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/heudiconv/tests/test_dicoms.py b/heudiconv/tests/test_dicoms.py index 092019b4..024969f2 100644 --- a/heudiconv/tests/test_dicoms.py +++ b/heudiconv/tests/test_dicoms.py @@ -21,7 +21,7 @@ parse_private_csa_header, ) -from .utils import TESTS_DATA_PATH +from .utils import TEST_DICOM_PATHS, TESTS_DATA_PATH # Public: Private DICOM tags DICOM_FIELDS_TO_TEST = {"ProtocolName": "tProtocolName"} @@ -180,21 +180,13 @@ def test_get_datetime_from_dcm_wo_dt() -> None: assert get_datetime_from_dcm(XA30_enhanced_dcm) is None -dicom_test_data = [ - (dw.wrapper_from_file(d_file), [d_file], op.basename(d_file)) - for d_file in glob(op.join(TESTS_DATA_PATH, "*.dcm")) -] - - -@pytest.mark.parametrize("mw,series_files,series_id", dicom_test_data) +@pytest.mark.parametrize("dcmfile", TEST_DICOM_PATHS) def test_create_seqinfo( - mw: dw.Wrapper, - series_files: list[str], - series_id: str, + dcmfile, ) -> None: - seqinfo = create_seqinfo(mw, series_files, series_id) - assert seqinfo.sequence_name != "" - pass + mw = dw.wrapper_from_file(dcmfile) + seqinfo = create_seqinfo(mw, [dcmfile], op.basename(dcmfile)) + assert seqinfo.sequence_name def test_get_reproducible_int() -> None: diff --git a/heudiconv/tests/utils.py b/heudiconv/tests/utils.py index 7db07ee0..950d3555 100644 --- a/heudiconv/tests/utils.py +++ b/heudiconv/tests/utils.py @@ -1,5 +1,6 @@ from __future__ import annotations +from glob import glob import logging import os.path as op from pathlib import Path @@ -9,6 +10,14 @@ HEURISTICS_PATH = op.join(heudiconv.heuristics.__path__[0]) TESTS_DATA_PATH = op.join(op.dirname(__file__), "data") +# Do relative to curdir to shorten in a typical application, +# and side-effect test that tests do not change curdir. +TEST_DICOM_PATHS = [ + op.relpath(x) + for x in glob(op.join(TESTS_DATA_PATH, "**/*.dcm"), recursive=True) + # exclude PhoenixDocuments + if "PhoenixDocument" not in x +] lgr = logging.getLogger(__name__) From 5c329be3b1e29565951febab559130611652a96e Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 16 Sep 2024 13:07:59 -0400 Subject: [PATCH 2/5] ENH: make test_get_reproducible_int also sweep through all dicom files --- heudiconv/tests/test_dicoms.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/heudiconv/tests/test_dicoms.py b/heudiconv/tests/test_dicoms.py index 024969f2..df7e7ba2 100644 --- a/heudiconv/tests/test_dicoms.py +++ b/heudiconv/tests/test_dicoms.py @@ -189,9 +189,8 @@ def test_create_seqinfo( assert seqinfo.sequence_name -def test_get_reproducible_int() -> None: - dcmfile = op.join(TESTS_DATA_PATH, "phantom.dcm") - +@pytest.mark.parametrize("dcmfile", TEST_DICOM_PATHS) +def test_get_reproducible_int(dcmfile) -> None: assert type(get_reproducible_int([dcmfile])) is int From f113da1f10f9c991f8143faaa8dfdea9b95fc76d Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 16 Sep 2024 13:17:34 -0400 Subject: [PATCH 3/5] RF: reduce duplication and log at DEBUG what was our assumption when we extracted sequence_name --- heudiconv/dicoms.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/heudiconv/dicoms.py b/heudiconv/dicoms.py index 6210f83d..cf4a84fd 100644 --- a/heudiconv/dicoms.py +++ b/heudiconv/dicoms.py @@ -94,15 +94,19 @@ def create_seqinfo( series_desc = get_typed_attr(dcminfo, "SeriesDescription", str, "") protocol_name = get_typed_attr(dcminfo, "ProtocolName", str, "") - if dcminfo.get([0x18, 0x24]): - # GE and Philips - sequence_name = dcminfo[0x18, 0x24].value - elif dcminfo.get([0x19, 0x109C]): - # Siemens - sequence_name = dcminfo[0x19, 0x109C].value - elif dcminfo.get([0x18, 0x9005]): - # Siemens XA - sequence_name = dcminfo[0x18, 0x9005].value + for k, m in ( + ([0x18, 0x24], "GE and Philips"), + ([0x19, 0x109C], "Siemens"), + ([0x18, 0x9005], "Siemens XA"), + ): + if v := dcminfo.get(k): + sequence_name = v.value + lgr.debug( + "Identified sequence name as %s coming from the %r family of MR scanners", + sequence_name, + m, + ) + break else: sequence_name = "" From 2efb747e8cecdb61980fa71f210ffb7086f2e41b Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Mon, 30 Sep 2024 18:19:51 -0400 Subject: [PATCH 4/5] Add type annotations Co-authored-by: Basile --- heudiconv/tests/test_dicoms.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/heudiconv/tests/test_dicoms.py b/heudiconv/tests/test_dicoms.py index df7e7ba2..6c20af84 100644 --- a/heudiconv/tests/test_dicoms.py +++ b/heudiconv/tests/test_dicoms.py @@ -182,7 +182,7 @@ def test_get_datetime_from_dcm_wo_dt() -> None: @pytest.mark.parametrize("dcmfile", TEST_DICOM_PATHS) def test_create_seqinfo( - dcmfile, + dcmfile: str, ) -> None: mw = dw.wrapper_from_file(dcmfile) seqinfo = create_seqinfo(mw, [dcmfile], op.basename(dcmfile)) @@ -190,7 +190,7 @@ def test_create_seqinfo( @pytest.mark.parametrize("dcmfile", TEST_DICOM_PATHS) -def test_get_reproducible_int(dcmfile) -> None: +def test_get_reproducible_int(dcmfile:str) -> None: assert type(get_reproducible_int([dcmfile])) is int From 7c24da04a1c4ed62722d08ba959300734d7781bd Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 1 Oct 2024 11:13:54 -0400 Subject: [PATCH 5/5] Add space to please lint --- heudiconv/tests/test_dicoms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/heudiconv/tests/test_dicoms.py b/heudiconv/tests/test_dicoms.py index 6c20af84..0a81942f 100644 --- a/heudiconv/tests/test_dicoms.py +++ b/heudiconv/tests/test_dicoms.py @@ -190,7 +190,7 @@ def test_create_seqinfo( @pytest.mark.parametrize("dcmfile", TEST_DICOM_PATHS) -def test_get_reproducible_int(dcmfile:str) -> None: +def test_get_reproducible_int(dcmfile: str) -> None: assert type(get_reproducible_int([dcmfile])) is int