Skip to content

BF+TST: Skip over missing CSA header elem instead of raising. #393

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.

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

Merged
merged 3 commits into from
Mar 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions nibabel/nicom/csareader.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,11 @@ def get_csa_header(dcm_data, csa_type='image'):
if section_start is None:
return None
element_no = section_start + element_offset
# Assume tag exists
tag = dcm_data[(0x29, element_no)]
try:
tag = dcm_data[(0x29, element_no)]
Copy link
Contributor

Choose a reason for hiding this comment

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

@moloney Can you add a comment about when this would fail, and why None is an OK value to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note about why it may fail. The docstring includes info about returning None.

except KeyError:
# The element could be missing due to anonymization
return None
return read(tag.value)


Expand Down
25 changes: 20 additions & 5 deletions nibabel/nicom/tests/test_csareader.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
""" Testing Siemens CSA header reader
"""
import sys
from os.path import join as pjoin
from copy import deepcopy
import gzip

import numpy as np
Expand All @@ -10,9 +12,12 @@

from nose.tools import (assert_true, assert_false, assert_equal, assert_raises)

from numpy.testing.decorators import skipif

from .test_dicomwrappers import (have_dicom, dicom_test,
IO_DATA_PATH, DATA, DATA_FILE)
if have_dicom:
from .test_dicomwrappers import pydicom
Copy link
Contributor

Choose a reason for hiding this comment

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

Another test imports this directly in the relevant test. Seems better to follow the same style; do you mind to move?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, better to use skipIf decorator on the test: skipif(not have_dicom)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up keeping the top level import and switching the other function to use it. This way the import logic doesn't get replicated and I can use the pydicom version in the skipif decorator.


CSA2_B0 = open(pjoin(IO_DATA_PATH, 'csa2_b0.bin'), 'rb').read()
CSA2_B1000 = open(pjoin(IO_DATA_PATH, 'csa2_b1000.bin'), 'rb').read()
Expand All @@ -30,11 +35,7 @@ def test_csa_header_read():
assert_true(csa.is_mosaic(hdr))
# Get a shallow copy of the data, lacking the CSA marker
# Need to do it this way because del appears broken in pydicom 0.9.7
try:
from dicom.dataset import Dataset
except ImportError:
from pydicom.dataset import Dataset
data2 = Dataset()
data2 = pydicom.dataset.Dataset()
for element in DATA:
if (element.tag.group, element.tag.elem) != (0x29, 0x10):
data2.add(element)
Expand Down Expand Up @@ -128,3 +129,17 @@ def test_ice_dims():
assert_equal(csa.get_ice_dims(csa_info),
ex_dims)
assert_equal(csa.get_ice_dims({}), None)


@dicom_test
@skipif(sys.version_info < (2,7) and pydicom.__version__ < '1.0',
'Known issue for python 2.6 and pydicom < 1.0')
def test_missing_csa_elem():
# Test that we get None instead of raising an Exception when the file has
# the PrivateCreator element for the CSA dict but not the element with the
# actual CSA header (perhaps due to anonymization)
dcm = deepcopy(DATA)
csa_tag = pydicom.dataset.Tag(0x29, 0x1010)
del dcm[csa_tag]
hdr = csa.get_csa_header(dcm, 'image')
assert_equal(hdr, None)