Merge branch 'rf/dcm2niix' into 'master'

dcm2niix updates

Closes fsl/fsleyes/fsleyes#153

See merge request fsl/fslpy!161

CHANGELOG.rst

@@ -25,6 +25,8 @@ Changed
 * The :meth:`.LabelAtlas.get` method has a new ``binary`` flag, allowing
   either a binary mask, or a mask with the original label value, to be
   returned.
+* The :mod:`.dicom` module has been updated to work with the latest version of
+  ``dcm2niix``.
 
 
 Deprecated

fsl/data/dicom.py

@@ -29,9 +29,12 @@ import               os
 import os.path    as op
 import subprocess as sp
 import               re
+import               sys
 import               glob
 import               json
+import               shlex
 import               logging
+import               binascii
 
 import nibabel    as nib
 
@@ -44,7 +47,16 @@ log = logging.getLogger(__name__)
 
 
 MIN_DCM2NIIX_VERSION = (1, 0, 2017, 12, 15)
-"""Minimum version of dcm2niix that is required for this module to work. """
+"""Minimum version of ``dcm2niix`` that is required for this module to work.
+"""
+
+
+CRC_DCM2NIIX_VERSION = (1, 0, 2019, 9, 2)
+"""For versions of ``dcm2niix`` orf this version or newer, the ``-n`` flag,
+used to convert a single DICOM series, requires that a CRC checksum
+identifying the series be passed (see the :func:`seriesCRC`
+function). Versions prior to this require the series number to be passed.
+"""
 
 
 class DicomImage(fslimage.Image):
@@ -80,9 +92,16 @@ class DicomImage(fslimage.Image):
 
 
 @memoize.memoize
-def enabled():
-    """Returns ``True`` if ``dcm2niix`` is present, and recent enough,
-    ``False`` otherwise.
+def installedVersion():
+    """Return a tuple describing the version of ``dcm2niix`` that is installed,
+    or ``None`` if dcm2niix cannot be found, or its version not parsed.
+
+    The returned tuple contains the following fields, all integers:
+      - Major version number
+      - Minor version number
+      - Year
+      - Month
+      - Day
     """
 
     cmd            = 'dcm2niix -h'
@@ -102,80 +121,120 @@ def enabled():
 
             match = re.match(versionPattern, word)
 
-            if match is None:
-                continue
+            if match is not None:
+                return (int(match.group('major')),
+                        int(match.group('minor')),
+                        int(match.group('year')),
+                        int(match.group('month')),
+                        int(match.group('day')))
 
-            installedVersion = (
-                int(match.group('major')),
-                int(match.group('minor')),
-                int(match.group('year')),
-                int(match.group('month')),
-                int(match.group('day')))
+    except Exception as e:
+        log.debug('Error parsing dcm2niix version string: {}'.format(e))
+    return None
 
-            # make sure installed version
-            # is equal to or newer than
-            # minimum required version
-            for iv, mv in zip(installedVersion, MIN_DCM2NIIX_VERSION):
-                if   iv > mv: return True
-                elif iv < mv: return False
 
-            # if we get here, versions are equal
-            return True
+def compareVersions(v1, v2):
+    """Compares two ``dcm2niix`` versions ``v1`` and ``v2``.  The versions are
+    assumed to be in the format returned by :func:`installedVersion`.
 
-    except Exception as e:
-        log.debug('Error parsing dcm2niix version string: {}'.format(e))
+    :returns: - 1 if ``v1`` is newer than ``v2``
+              - -1 if ``v1`` is older than ``v2``
+              - 0 if ``v1`` the same as ``v2``.
+    """
+
+    for iv1, iv2 in zip(v1, v2):
+        if   iv1 > iv2: return  1
+        elif iv1 < iv2: return -1
+    return 0
 
-    return False
+
+def enabled():
+    """Returns ``True`` if ``dcm2niix`` is present, and recent enough,
+    ``False`` otherwise.
+    """
+    installed = installedVersion()
+    required  = MIN_DCM2NIIX_VERSION
+    return ((installed is not None) and
+            (compareVersions(installed, required) >= 0))
 
 
 def scanDir(dcmdir):
-    """Uses ``dcm2niix`` to scans the given DICOM directory, and returns a
-    list of dictionaries, one for each data series that was identified.
-    Each dictionary is populated with some basic metadata about the series.
+    """Uses the ``dcm2niix -b o`` option to generate a BIDS sidecar JSON
+    file for each series in the given DICOM directory. Reads them all in,
+    and returns them as a sequence of dicts.
 
-    :arg dcmdir: Directory containing DICOM files.
+    Some additional metadata is added to each dictionary:
+     - ``DicomDir``: The absolute path to ``dcmdir``
 
-    :returns:    A list of dictionaries, each containing metadata about
-                 one DICOM data series.
+    :arg dcmdir: Directory containing DICOM series
+
+    :returns:    A list of dicts, each containing the BIDS sidecar JSON
+                 metadata for one DICOM series.
     """
 
     if not enabled():
         raise RuntimeError('dcm2niix is not available or is too old')
 
-    dcmdir      = op.abspath(dcmdir)
-    cmd         = 'dcm2niix -b o -ba n -f %s -o . {}'.format(dcmdir)
-    snumPattern = re.compile('^[0-9]+')
+    dcmdir = op.abspath(dcmdir)
+    cmd    = 'dcm2niix -b o -ba n -f %s -o . "{}"'.format(dcmdir)
+    series = []
 
     with tempdir.tempdir() as td:
 
         with open(os.devnull, 'wb') as devnull:
-            sp.call(cmd.split(), stdout=devnull, stderr=devnull)
+            sp.call(shlex.split(cmd), stdout=devnull, stderr=devnull)
 
         files = glob.glob(op.join(td, '*.json'))
 
         if len(files) == 0:
             return []
 
-        # sort numerically by series number if possible
-        try:
-            def sortkey(f):
-                match = re.match(snumPattern, f)
-                snum  = int(match.group(0))
-                return snum
-
-            files = sorted(files, key=sortkey)
-
-        except Exception:
-            files = sorted(files)
-
-        series = []
         for fn in files:
             with open(fn, 'rt') as f:
-                meta = json.load(f)
+                meta             = json.load(f)
                 meta['DicomDir'] = dcmdir
                 series.append(meta)
 
-        return series
+    # sort by series number
+    def key(s):
+        return s.get('SeriesNumber', sys.maxsize)
+
+    series = list(sorted(series, key=key))
+
+    return series
+
+
+def seriesCRC(series):
+    """Calculate a checksum string of the given DICOM series.
+
+    The returned string is of the form::
+
+         SeriesCRC[.echonumber]
+
+    Where ``SeriesCRC`` is an unsigned integer which is the CRC32
+    checksum of the ``SeriesInstanceUID``, and ``echonumber`` is
+    the ``EchoNumber`` of the series - this is only present for
+    multi-echo data, where the series is from the second or subsequent
+    echos.
+
+    :arg series: Dict containing BIDS metadata about a DICOM series,
+                 as returned by :func:`scanDir`.
+
+    :returns:    String containing a CRC32 checksum for the series.
+    """
+
+    uid  = series.get('SeriesInstanceUID', None)
+    echo = series.get('EchoNumber',        None)
+
+    if uid is None:
+        return None
+
+    crc32 = str(binascii.crc32(uid.encode()))
+
+    if echo is not None and echo > 1:
+        crc32 = '{}.{}'.format(crc32, echo)
+
+    return crc32
 
 
 def loadSeries(series):
@@ -192,15 +251,28 @@ def loadSeries(series):
     if not enabled():
         raise RuntimeError('dcm2niix is not available or is too old')
 
-    dcmdir = series['DicomDir']
-    snum   = series['SeriesNumber']
-    desc   = series['SeriesDescription']
-    cmd    = 'dcm2niix -b n -f %s -z n -o . -n {} {}'.format(snum, dcmdir)
+    dcmdir  = series['DicomDir']
+    snum    = series['SeriesNumber']
+    desc    = series['SeriesDescription']
+    version = installedVersion()
+
+    # Newer versions of dcm2niix
+    # require a CRC to identify
+    # series
+    if compareVersions(version, CRC_DCM2NIIX_VERSION) >= 0:
+        ident = seriesCRC(series)
+
+    # Older versions require
+    # the series number
+    else:
+        ident = snum
+
+    cmd = 'dcm2niix -b n -f %s -z n -o . -n "{}" "{}"'.format(ident, dcmdir)
 
     with tempdir.tempdir() as td:
 
         with open(os.devnull, 'wb') as devnull:
-            sp.call(cmd.split(), stdout=devnull, stderr=devnull)
+            sp.call(shlex.split(cmd), stdout=devnull, stderr=devnull)
 
         files  = glob.glob(op.join(td, '{}*.nii'.format(snum)))
         images = [nib.load(f, mmap=False) for f in files]

requirements-dev.txt

 sphinx
 sphinx_rtd_theme
-mock
 coverage
 pytest
 pytest-cov

tests/test_deprecated.py

@@ -7,12 +7,22 @@ import pytest
 import fsl.utils.deprecated as deprecated
 
 
-# the line number of the warning is hard coded in
-# the unit tests below. Don't change the line number!
+# these  get updated in the relevant functions
+WARNING_LINE_NUMBER = None
+DEPRECATED_LINE_NUMBER = None
+
+def _linenum(pattern):
+    with open(__file__, 'rt') as f:
+        for i, line in enumerate(f.readlines(), 1):
+            if pattern in line:
+                return i
+    return -1
+
+
 def emit_warning():
     deprecated.warn('blag', vin='1.0.0', rin='2.0.0', msg='yo')
-
-WARNING_LINE_NUMBER = 13
+    global WARNING_LINE_NUMBER
+    WARNING_LINE_NUMBER = _linenum('deprecated.warn(\'blag\'')
 
 
 @deprecated.deprecated(vin='1.0.0', rin='2.0.0', msg='yo')
@@ -20,9 +30,9 @@ def depfunc():
     pass
 
 def call_dep_func():
-    depfunc()
-
-DEPRECATED_LINE_NUMBER = 23
+    depfunc()  # mark
+    global DEPRECATED_LINE_NUMBER
+    DEPRECATED_LINE_NUMBER = _linenum('depfunc()  # mark')
 
 
 def _check_warning(w, name, lineno):

tests/test_dicom.py

 #!/usr/bin/env python
 #
-# test_dicom.py -
-#
-# Author: Paul McCarthy <pauldmccarthy@gmail.com>
+# These tests require an internet connection, and will only work on linux.
 #
 
-import os.path as op
-import            tarfile
+import os.path        as op
+import                   os
+import functools      as ft
+import subprocess     as sp
+import                   tarfile
+import                   zipfile
+import                   random
+import                   string
+import                   binascii
+import                   contextlib
+import urllib.request as request
+from   unittest import   mock
 
-import mock
 import pytest
 
 import fsl.data.dicom    as fsldcm
@@ -21,14 +28,35 @@ datadir = op.join(op.dirname(__file__), 'testdata')
 pytestmark = pytest.mark.dicomtest
 
 
-def setup_module():
+@contextlib.contextmanager
+def install_dcm2niix(version='1.0.20190902'):
+    filenames = {
+        '1.0.20190902' : 'v1.0.20190902/dcm2niix_lnx.zip',
+        '1.0.20190410' : 'v1.0.20190410/dcm2niix_11-Apr-2019_lnx.zip',
+        '1.0.20181125' : 'v1.0.20181125/dcm2niix_25-Nov-2018_lnx.zip',
+        '1.0.20171017' : 'v1.0.20171017/dcm2niix_18-Oct-2017_lnx.zip',
+    }
+    prefix = 'https://github.com/rordenlab/dcm2niix/releases/download/'
+    url    = prefix + filenames[version]
 
-    if not fsldcm.enabled():
-        raise RuntimeError('dcm2niix is not present - tests cannot be run')
+    with tempdir.tempdir() as td:
+        request.urlretrieve(url, 'dcm2niix.zip')
 
+        with zipfile.ZipFile('dcm2niix.zip', 'r') as f:
+            f.extractall('.')
+
+        os.chmod(op.join(td, 'dcm2niix'), 0o755)
+
+        path = op.pathsep.join((op.abspath('.'), os.environ['PATH']))
+
+        with mock.patch.dict('os.environ', {'PATH' : path}):
+            try:
+                yield
+            finally:
+                fsldcm.installedVersion.invalidate()
 
-def test_disabled():
 
+def test_disabled():
     with mock.patch('fsl.data.dicom.enabled', return_value=False):
         with pytest.raises(RuntimeError):
             fsldcm.scanDir('.')
@@ -36,20 +64,37 @@ def test_disabled():
             fsldcm.loadSeries({})
 
 
+
+def test_installedVersion():
+    tests = [
+        ('1.0.20190902', (1, 0, 2019, 9, 2)),
+        ('1.0.20181125', (1, 0, 2018, 11, 25)),
+        ('1.0.20171017', (1, 0, 2017, 10, 17))]
+
+    for version, expect in tests:
+        fsldcm.installedVersion.invalidate()
+        with install_dcm2niix(version):
+            got = fsldcm.installedVersion()
+            assert got == expect
+
+
+
 def test_enabled():
 
     try:
-        fsldcm.enabled.invalidate()
-        assert fsldcm.enabled()
-        fsldcm.enabled.invalidate()
+        with install_dcm2niix('1.0.20190902'):
+            fsldcm.installedVersion.invalidate()
+            assert fsldcm.enabled()
+
         # test dcm2niix not present
         with mock.patch('subprocess.check_output',
                         side_effect=Exception()):
+            fsldcm.installedVersion.invalidate()
             assert not fsldcm.enabled()
 
-
         # test presence of different versions
         tests = [(b'version v2.1.20191212', True),
+                 (b'version v1.0.20190902', True),
                  (b'version v1.0.20171216', True),
                  (b'version v1.0.20171215', True),
                  (b'version v1.0.20171214', False),
@@ -59,19 +104,19 @@ def test_enabled():
                  (b'version blurgh',        False)]
 
         for verstr, expected in tests:
-            fsldcm.enabled.invalidate()
+            fsldcm.installedVersion.invalidate()
             with mock.patch('subprocess.check_output', return_value=verstr):
                 assert fsldcm.enabled() == expected
 
     finally:
-        fsldcm.enabled.invalidate()
+        fsldcm.installedVersion.invalidate()
 
 
 def test_scanDir():
 
-    with tempdir.tempdir() as td:
+    with install_dcm2niix():
 
-        series = fsldcm.scanDir(td)
+        series = fsldcm.scanDir('.')
         assert len(series) == 0
 
         datafile = op.join(datadir, 'example_dicom.tbz2')
@@ -79,42 +124,69 @@ def test_scanDir():
         with tarfile.open(datafile) as f:
             f.extractall()
 
-        series = fsldcm.scanDir(td)
-        assert len(series) == 3
+        series = fsldcm.scanDir('.')
+        assert len(series) == 2
 
         for s in series:
             assert (s['PatientName'] == 'MCCARTHY_PAUL' or
                     s['PatientName'] == 'MCCARTHY_PAUL_2')
 
 
+def test_sersiesCRC():
+    RANDOM = object()
+    tests = [
+        ({'SeriesInstanceUID' : 'hello-world'},            '2983461467'),
+        ({'SeriesInstanceUID' : RANDOM, 'EchoNumber' : 0}, RANDOM),
+        ({'SeriesInstanceUID' : RANDOM, 'EchoNumber' : 1}, RANDOM),
+        ({'SeriesInstanceUID' : RANDOM, 'EchoNumber' : 2}, RANDOM),
+        ({'SeriesInstanceUID' : RANDOM, 'EchoNumber' : 3}, RANDOM),
+    ]
+
+    for series, expect in tests:
+        series = dict(series)
+        if expect is RANDOM:
+            expect = ''.join([random.choice(string.ascii_letters + string.digits)
+                              for i in range(30)])
+            series['SeriesInstanceUID'] = expect
+            expect = str(binascii.crc32(expect.encode()))
+        echo = series.get('EchoNumber', 0)
+        if echo > 1:
+            expect += '.{}'.format(echo)
+        assert fsldcm.seriesCRC(series) == expect
+
+
 def test_loadSeries():
 
-    with tempdir.tempdir() as td:
+    # test a pre-CRC and a post-CRC version
+    for version in ('1.0.20190410', '1.0.20190902'):
 
-        datafile = op.join(datadir, 'example_dicom.tbz2')
+        with install_dcm2niix(version):
 
-        with tarfile.open(datafile) as f:
-            f.extractall()
+            datafile = op.join(datadir, 'example_dicom.tbz2')
+
+            with tarfile.open(datafile) as f:
+                f.extractall()
 
-        series   = fsldcm.scanDir(td)
-        expShape = (512, 512, 1)
-        explens  = [1, 2]
+            dcmdir   = os.getcwd()
+            series   = fsldcm.scanDir(dcmdir)
+            expShape = (512, 512, 1)
+            explens  = [1, 1]
 
-        for s, explen in zip(series, explens):
+            for s, explen in zip(series, explens):
 
-            imgs = fsldcm.loadSeries(s)
+                imgs = fsldcm.loadSeries(s)
 
-            assert len(imgs) == explen
+                assert len(imgs) == explen
 
-            for img in imgs:
+                for img in imgs:
 
-                assert img.dicomDir               == td
-                assert img.shape                  == expShape
-                assert img[:].shape               == expShape
-                assert img.getMeta('PatientName') == 'MCCARTHY_PAUL' or \
-                       img.getMeta('PatientName') == 'MCCARTHY_PAUL_2'
-                assert 'PatientName'                      in img.metaKeys()
-                assert 'MCCARTHY_PAUL'                    in img.metaValues() or \
-                       'MCCARTHY_PAUL_2'                  in img.metaValues()
-                assert ('PatientName', 'MCCARTHY_PAUL')   in img.metaItems() or \
-                       ('PatientName', 'MCCARTHY_PAUL_2') in img.metaItems()
+                    assert img.dicomDir               == dcmdir
+                    assert img.shape                  == expShape
+                    assert img[:].shape               == expShape
+                    assert img.getMeta('PatientName') == 'MCCARTHY_PAUL' or \
+                           img.getMeta('PatientName') == 'MCCARTHY_PAUL_2'
+                    assert 'PatientName'                      in img.metaKeys()
+                    assert 'MCCARTHY_PAUL'                    in img.metaValues() or \
+                           'MCCARTHY_PAUL_2'                  in img.metaValues()
+                    assert ('PatientName', 'MCCARTHY_PAUL')   in img.metaItems() or \
+                           ('PatientName', 'MCCARTHY_PAUL_2') in img.metaItems()