Skip to content

ENH: Add "element" containers and make dicom wrappers compatible #416

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

Closed
wants to merge 9 commits into from
Closed
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
189 changes: 189 additions & 0 deletions nibabel/elemcont.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
# vi: set ft=python sts=4 ts=4 sw=4 et:
'''Containers that provide easy access to the values of nested elements

These containers are for storing "elements" which have both a core data value
as well as some additional meta data. When indexing into these containers it is
this core value that is returned, which allows for much cleaner and more
readable access to nested structures.

Each object stored in these containers must have an attribute `value` which
provides the core data value for the element. To get the element object itself
the `get_elem` method must be used.
'''
from collections import MutableMapping, MutableSequence

from .externals import OrderedDict


class MetaElem(object):
'''Basic element type has a `value` and a `meta` attribute.'''
def __init__(self, value, meta=None):
self.value = value
self.meta = {} if meta is None else meta


class InvalidElemError(Exception):
'''The object being added to the container doesn't have a `value` attribute
'''
def __init__(self, invalid_val):
message = ("Provided value '%s' of type %s does not have a 'value' "
"attribute" % (invalid_val, type(invalid_val)))
super(InvalidElemError, self).__init__(message)


class ElemDict(MutableMapping):
'''Ordered dict-like providing easy access to nested elements

Each value added to the dict must in turn have a `value` attribute, which
is what is returned by subsequent calls to `__getitem__`. To get the
element itself use the `get_elem` method.
'''

def __init__(self, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Why *args here rather than val or something?

Copy link
Member

Choose a reason for hiding this comment

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

Do I understand right that args[0] can be any of

  • ElemDict
  • dict
  • sequence of (key, value) sequences ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is trying to replicate the behavior of the dict constructor. You are correct that args[0] can be dict-like of a list tuples. Either way the elements need to have a value attribute.

if len(args) > 1:
raise TypeError("At most one arg expected, got %d" % len(args))
self._elems = OrderedDict()
if len(args) == 1:
arg = args[0]
if hasattr(arg, 'get_elem'):
it = ((k, arg.get_elem(k)) for k in arg)
elif hasattr(arg, 'items'):
it = arg.items()
else:
it = arg
for key, val in it:
self[key] = val
for key, val in kwargs.items():
self[key] = val

def __getitem__(self, key):
return self._elems[key].value

def __setitem__(self, key, val):
if not hasattr(val, 'value'):
raise InvalidElemError(val)
self._elems[key] = val

def __delitem__(self, key):
Copy link
Member

Choose a reason for hiding this comment

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

You could avoid these three methods by subclassing dict - any reason not to?

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 believe that subclassing MutableMapping is preferred for more "custom" dict-like classes, rather than just extending the API of a standard dict. In particular, other standard dict methods won't call your custom __getitem__ and __setitem__ methods.

Copy link
Member

Choose a reason for hiding this comment

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

At least this:

class MyDict(dict):
    def __getitem__(self, key):
        return 'foo'

    def my_method(self):
        return self['something']

d = MyDict()
print(d.my_method())

gives foo - or did you mean something else?

I guess MutableMapping doesn't have things like __len__ etc?

del self._elems[key]

def __iter__(self):
return iter(self._elems)

def __len__(self):
return len(self._elems)

def __repr__(self):
return ('ElemDict(%s)' %
', '.join(['%r=%r' % x for x in self.items()]))

def update(self, other):
if hasattr(other, 'get_elem'):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add iter_elems method to return key, element pairs, and use that?

for key in other:
self[key] = other.get_elem(key)
else:
for key, elem in other.items():
self[key] = elem

def get_elem(self, key):
return self._elems[key]


class ElemList(MutableSequence):
'''A list-like container providing easy access to nested elements

Each value added to the list must in turn have a `value` attribute, which
is what is returned by subsequent calls to `__getitem__`. To get the
element itself use the `get_elem` method.
'''
def __init__(self, data=None):
self._elems = list()
if data is None:
return
if isinstance(data, self.__class__):
for idx in range(len(data)):
self.append(data.get_elem(idx))
else:
for elem in data:
self.append(elem)

def _tuple_from_slice(self, slc):
'''Get (start, end, step) tuple from slice object.
'''
(start, end, step) = slc.indices(len(self))
# Replace (0, -1, 1) with (0, 0, 1) (misfeature in .indices()).
if step == 1:
if end < start:
end = start
step = None
if slc.step is None:
step = None
return (start, end, step)

def __getitem__(self, idx):
if isinstance(idx, slice):
return ElemList(self._elems[idx])
else:
return self._elems[idx].value

def __setitem__(self, idx, val):
if isinstance(idx, slice):
(start, end, step) = self._tuple_from_slice(idx)
if step is None:
# Normal slice
for j, assign_val in enumerate(val):
self.insert(start + j, assign_val)
return
# Extended slice
indices = range(start, end, step)
if len(val) != len(indices):
raise ValueError(('attempt to assign sequence of size %d' +
' to extended slice of size %d') %
(len(value), len(indices)))
for j, assign_val in enumerate(val):
self.insert(indices[j], assign_val)
else:
self.insert(idx, val)

def __delitem__(self, idx):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe subclass list to avoid these guys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, I think MutableSequence is preferred in this situation.

del self._elems[idx]

def __len__(self):
return len(self._elems)

def __repr__(self):
return ('ElemList([%s])' % ', '.join(['%r' % x for x in self]))

def __add__(self, other):
result = self.__class__(self)
if isinstance(other, self.__class__):
Copy link
Member

Choose a reason for hiding this comment

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

How about an iter_elems method, use that here and in __init__ ?

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 agree iter_elems is good for the ElemDict class, but this should really be using enumerate right?

Edit: Oops, that would produce values not elems... Some kind of method for this sounds good, maybe enum_elems?

Edit2: Doh! I just realized you meant to just produce elements not (idx, element) tuples... Maybe give it different name from the dict method?

Copy link
Member

Choose a reason for hiding this comment

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

How about iter_mitems for the dict and iter_melems for the list?

for idx in range(len(other)):
result.append(other.get_elem(idx))
else:
for e in other:
result.append(e)
return result

def __radd__(self, other):
result = self.__class__(other)
for idx in range(len(self)):
result.append(self.get_elem(idx))
return result

def __iadd__(self, other):
if isinstance(other, self.__class__):
for idx in range(len(other)):
self.append(other.get_elem(idx))
else:
for e in other:
self.append(e)
return self

def insert(self, idx, val):
if not hasattr(val, 'value'):
raise InvalidElemError(val)
self._elems.insert(idx, val)

def get_elem(self, idx):
return self._elems[idx]
13 changes: 13 additions & 0 deletions nibabel/nicom/dicomwrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ class Wrapper(object):
* is_same_series(other)
* __getitem__ : return attributes from `dcm_data`
* get(key[, default]) - as usual given __getitem__ above
* get_elem(key) - Provide full DICOM element instead of just value
* __iter__ - Iterate over DICOM keys available in this data set

Attributes and things that look like attributes:

Expand Down Expand Up @@ -293,6 +295,17 @@ def get(self, key, default=None):
""" Get values from underlying dicom data """
return self.dcm_data.get(key, default)

def get_elem(self, key):
""" Get DICOM element instead of just the value """
tag = tag_for_keyword(key)
if tag is None or tag not in self.dcm_data:
raise KeyError('"%s" not in self.dcm_data' % key)
return self.dcm_data.get(tag)

def __iter__(self):
""" Iterate over available DICOM keywords """
return iter(self.dcm_data.dir())

@deprecate_with_version('get_affine method is deprecated.\n'
'Please use the ``img.affine`` property '
'instead.',
Expand Down
12 changes: 12 additions & 0 deletions nibabel/nicom/tests/test_dicomwrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ def test_wrappers():
assert dw.get('AcquisitionNumber') is None
with pytest.raises(KeyError):
dw['not an item']
with pytest.raises(KeyError):
dw.get_elem('not an item')
with pytest.raises(didw.WrapperError):
dw.get_data()
with pytest.raises(didw.WrapperError):
Expand All @@ -93,8 +95,18 @@ def test_wrappers():
dw = maker(DATA)
assert dw.get('InstanceNumber') == 2
assert dw.get('AcquisitionNumber') == 2
elem = dw.get_elem('InstanceNumber')
assert type(elem) == pydicom.dataelem.DataElement
assert elem.value == 2
assert elem.VR == 'IS'
with pytest.raises(KeyError):
dw['not an item']
with pytest.raises(KeyError):
dw.get_elem('not an item')
# Test we get a key error when using real DICOM keyword that isn't in
# this dataset
with pytest.raises(KeyError):
dw.get_elem('ShutterShape')
for maker in (didw.MosaicWrapper, didw.wrapper_from_data):
dw = maker(DATA)
assert dw.is_mosaic
Expand Down
95 changes: 95 additions & 0 deletions nibabel/tests/test_elemcont.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
""" Testing element containers
"""
from __future__ import print_function

from ..elemcont import MetaElem, ElemDict, ElemList, InvalidElemError

import pytest


def test_elemdict():
# Test ElemDict class
e = ElemDict()
with pytest.raises(InvalidElemError):
e['some'] = 'thing'
assert list(e.keys()) == []
elem = MetaElem('thing')
e['some'] = elem
assert list(e.keys()) == ['some']
assert e['some'] == 'thing'
assert e.get_elem('some') == elem

# Test constructor
with pytest.raises(InvalidElemError):
ElemDict(dict(some='thing'))
e = ElemDict(dict(some=MetaElem('thing')))
assert list(e.keys()) == ['some']
assert e['some'] == 'thing'
e = ElemDict(some=MetaElem('thing'))
assert list(e.keys()) == ['some']
assert e['some'] == 'thing'
e2 = ElemDict(e)
assert list(e2.keys()) == ['some']
assert e2['some'] == 'thing'


def test_elemdict_update():
e1 = ElemDict(dict(some=MetaElem('thing')))
e1.update(dict(hello=MetaElem('world')))
assert list(e1.items()) == [('some', 'thing'), ('hello', 'world')]
e1 = ElemDict(dict(some=MetaElem('thing')))
e2 = ElemDict(dict(hello=MetaElem('world')))
e1.update(e2)
assert list(e1.items()) == [('some', 'thing'), ('hello', 'world')]


def test_elemlist():
# Test ElemList class
el = ElemList()
assert len(el) == 0
with pytest.raises(InvalidElemError):
el.append('something')
elem = MetaElem('something')
el.append(elem)
assert len(el) == 1
assert el[0] == 'something'
assert el.get_elem(0) == elem
assert [x for x in el] == ['something']

# Test constructor
with pytest.raises(InvalidElemError):
ElemList(['something'])
el = ElemList([elem])
assert len(el) == 1
assert el[0] == 'something'
assert el.get_elem(0) == elem
el2 = ElemList(el)
assert len(el2) == 1
assert el2[0] == 'something'
assert el2.get_elem(0) == elem


def test_elemlist_slicing():
el = ElemList()
el[5:6] = [MetaElem('hello'), MetaElem('there'), MetaElem('world')]
assert [x for x in el] == ['hello', 'there', 'world']
assert isinstance(el[:2], ElemList)
assert [x for x in el[:2]] == ['hello', 'there']


def test_elemlist_add():
res = ElemList([MetaElem('hello'), MetaElem('there')]) + ElemList([MetaElem('world')])
assert isinstance(res, ElemList)
assert [x for x in res] == ['hello', 'there', 'world']
res = ElemList([MetaElem('hello'), MetaElem('there')]) + [MetaElem('world')]
assert isinstance(res, ElemList)
assert [x for x in res] == ['hello', 'there', 'world']
res = [MetaElem('hello'), MetaElem('there')] + ElemList([MetaElem('world')])
assert isinstance(res, ElemList)
assert [x for x in res] == ['hello', 'there', 'world']
res = ElemList([MetaElem('hello'), MetaElem('there')])
res += [MetaElem('world')]
assert [x for x in res] == ['hello', 'there', 'world']
res = ElemList([MetaElem('hello'), MetaElem('there')])
res += ElemList([MetaElem('world')])
assert [x for x in res] == ['hello', 'there', 'world']