Repository: pydicom/pydicom

Issue:
Is this a bug in write_file?
_From [bton...@gmail.com](https://code.google.com/u/100845324794297671624/) on November 09, 2013 13:17:27_

What steps will reproduce the problem? (code stub attached)
1. Modify a valid dvh value: ds1.DVHs[0].DVHData[0] = NewFirstPoint
2. Save using ds1.write_file, and read back again into new ds2
3. Read-in data is not updated correctly; ds1.DVHs[0].DVHData[0] <> ds2.DVHS[0].DVHData[0]. The data stored reflects the ORIGINAL data in ds1 (which means that write_file is getting the data from a shadowed copy of the data? Subtle.) What is the expected output? What do you see instead? Sample code attached. Used a valid RT Dose file with DVH's as a starting point. Later, tried creating DVH's from scratch, and found a similar behavior. Once DVHData is appended to DVHs[index], elements of DVHData that are changed programmatically are not saved with write_file. 
Is this a bug, or have I missed a step? What version of the product are you using? Tried this with both 0.9.6 and 0.9.7 **_NOTE**_: any text or attached files posted with the issue can be viewed by anyone. You are solely responsible to ensure that they contain no confidential information of any kind. Please provide any additional information below. CODE STUB ATTACHED

**Attachment:** [dvh_rw_error.py](http://code.google.com/p/pydicom/issues/detail?id=135)

_Original issue: http://code.google.com/p/pydicom/issues/detail?id=135_


Discussion:
pep8speaks: Hello @mrbean-bremen! Thanks for submitting the PR.





 - In the file [`pydicom/multival.py`](https://github.com/pydicom/pydicom/blob/83d0e17f8740bf674b08eeec963cbf1d9dd99013/pydicom/multival.py), following are the PEP8 issues :

> [Line 45:13](https://github.com/pydicom/pydicom/blob/83d0e17f8740bf674b08eeec963cbf1d9dd99013/pydicom/multival.py#L45): [E731](https://duckduckgo.com/?q=pep8%20E731) do not assign a lambda expression, use a def




scaramallion: Looks good to me.
massich: Still not tested. lines 294, 322, 352 in file `pydicom/filewriter.py` are not covered. 

Two options:
a) either create 3 tests to test `write_TM`, `write_DT` and `write_DA` respectively by passing `data_element` with multiple values insde and ensuring that the written file is correct
Or b) refactor the function and add a single test to ensure that the string is properly generated
mrbean-bremen: Note: I have squashed the relevant commits and separated the pep8 commit, so this can be rebased.
mrbean-bremen: @darcymason, @massich - please check if this is sufficient.
mrbean-bremen: @massich - I added a test to cover the missing line
massich: LGTM. (well... travis is doing wired stuff, but I think that it might be that they just changed something and they are rebuilding the wheels in gdcm. Let them sleep through it, trigger travis again and I think would be all green)
mrbean-bremen: Note: I have removed the PEP-8 commit (good that I had separated it...), as that has already been done by @scaramallion.
mrbean-bremen: @darcymason, @massich - anything left to do here?
darcymason: LGTM to me, other than my one minor test comment.  I'll try to check in later today to merge, or @scaramallion can do so if no further comments from @massich.

Patch:
diff --git a/pydicom/dataelem.py b/pydicom/dataelem.py
index 36315e0ba..ea60bc4d0 100644
--- a/pydicom/dataelem.py
+++ b/pydicom/dataelem.py
@@ -14,6 +14,8 @@ A DataElement has a tag,
 from __future__ import absolute_import
 from collections import namedtuple
 
+from pydicom.multival import MultiValue
+
 from pydicom.charset import default_encoding
 
 from pydicom import config  # don't import datetime_conversion directly
@@ -230,10 +232,7 @@ class DataElement(object):
         except AttributeError:  # not a list
             return self._convert(val)
         else:
-            returnvalue = []
-            for subval in val:
-                returnvalue.append(self._convert(subval))
-            return returnvalue
+            return MultiValue(lambda x: self._convert(x), val)
 
     def _convert(self, val):
         """Convert `val` to an appropriate type for the element's VR."""
diff --git a/pydicom/filewriter.py b/pydicom/filewriter.py
index 0ccbf1506..17286a413 100644
--- a/pydicom/filewriter.py
+++ b/pydicom/filewriter.py
@@ -10,6 +10,7 @@ from pydicom.charset import default_encoding, text_VRs, convert_encodings
 from pydicom.datadict import keyword_for_tag
 from pydicom.dataset import Dataset
 from pydicom.filebase import DicomFile, DicomFileLike
+from pydicom.multival import MultiValue
 from pydicom.tag import Tag, ItemTag, ItemDelimiterTag, SequenceDelimiterTag
 from pydicom.tagtools import tag_in_exception
 from pydicom.uid import (PYDICOM_IMPLEMENTATION_UID, ImplicitVRLittleEndian,
@@ -200,9 +201,14 @@ def write_UI(fp, data_element):
     write_string(fp, data_element, '\0')  # pad with 0-byte to even length
 
 
+def _is_multi_value(val):
+    """Return True if `val` is a multi-value container."""
+    return isinstance(val, (MultiValue, list, tuple))
+
+
 def multi_string(val):
     """Put a string together with delimiter if has more than one value"""
-    if isinstance(val, (list, tuple)):
+    if _is_multi_value(val):
         # \ is escape chr, so "\\" gives single backslash
         return "\\".join(val)
     else:
@@ -248,7 +254,7 @@ def write_number_string(fp, data_element, padding=' '):
     # file
     val = data_element.value
 
-    if isinstance(val, (list, tuple)):
+    if _is_multi_value(val):
         val = "\\".join((x.original_string
                          if hasattr(x, 'original_string') else str(x)
                          for x in val))
@@ -281,7 +287,7 @@ def write_DA(fp, data_element, padding=' '):
     if isinstance(val, (str, compat.string_types)):
         write_string(fp, data_element, padding)
     else:
-        if isinstance(val, (list, tuple)):
+        if _is_multi_value(val):
             val = "\\".join((x if isinstance(x, (str, compat.string_types))
                              else _format_DA(x) for x in val))
         else:
@@ -309,7 +315,7 @@ def write_DT(fp, data_element, padding=' '):
     if isinstance(val, (str, compat.string_types)):
         write_string(fp, data_element, padding)
     else:
-        if isinstance(val, (list, tuple)):
+        if _is_multi_value(val):
             val = "\\".join((x if isinstance(x, (str, compat.string_types))
                              else _format_DT(x) for x in val))
         else:
@@ -339,7 +345,7 @@ def write_TM(fp, data_element, padding=' '):
     if isinstance(val, (str, compat.string_types)):
         write_string(fp, data_element, padding)
     else:
-        if isinstance(val, (list, tuple)):
+        if _is_multi_value(val):
             val = "\\".join((x if isinstance(x, (str, compat.string_types))
                              else _format_TM(x) for x in val))
         else:
diff --git a/pydicom/multival.py b/pydicom/multival.py
index 0829ac76a..5bc72b0d4 100644
--- a/pydicom/multival.py
+++ b/pydicom/multival.py
@@ -8,9 +8,10 @@ or any list of items that must all be the same type.
 #    See the file LICENSE included with this distribution, also
 #    available at https://github.com/pydicom/pydicom
 #
+from collections import MutableSequence
 
 
-class MultiValue(list):
+class MultiValue(MutableSequence):
     """Class to hold any multi-valued DICOM value,
     or any list of items that are all of the same type.
 
@@ -38,39 +39,44 @@ class MultiValue(list):
         """
         from pydicom.valuerep import DSfloat, DSdecimal, IS
 
-        self.type_constructor = type_constructor
-
-        if isinstance(type_constructor, (DSfloat, IS, DSdecimal)):
-            converted_list = [
-                type_constructor(x) if x != '' else x for x in iterable
-            ]
-        else:
-            converted_list = [type_constructor(x) for x in iterable]
-        super(MultiValue, self).__init__(converted_list)
+        def number_string_type_constructor(x):
+            return self.type_constructor(x) if x != '' else x
 
-    def append(self, val):
-        super(MultiValue, self).append(self.type_constructor(val))
-
-    def __deepcopy__(self, memo):
-        return MultiValue(self.type_constructor, self)
-
-    def extend(self, list_of_vals):
-        super(MultiValue, self).extend((self.type_constructor(x)
-                                        for x in list_of_vals))
+        self._list = list()
+        self.type_constructor = type_constructor
+        if type_constructor in (DSfloat, IS, DSdecimal):
+            type_constructor = number_string_type_constructor
+        for x in iterable:
+            self._list.append(type_constructor(x))
 
     def insert(self, position, val):
-        super(MultiValue, self).insert(position, self.type_constructor(val))
+        self._list.insert(position, self.type_constructor(val))
 
     def __setitem__(self, i, val):
         """Set an item of the list, making sure it is of the right VR type"""
         if isinstance(i, slice):
-            val = [self.type_constructor(x) for x in val]
+            val = [self.type_constructor(v) for v in val]
+            self._list.__setitem__(i, val)
         else:
-            val = self.type_constructor(val)
-        super(MultiValue, self).__setitem__(i, val)
+            self._list.__setitem__(i, self.type_constructor(val))
 
     def __str__(self):
         lines = [str(x) for x in self]
         return "['" + "', '".join(lines) + "']"
 
     __repr__ = __str__
+
+    def __len__(self):
+        return len(self._list)
+
+    def __getitem__(self, index):
+        return self._list[index]
+
+    def __delitem__(self, index):
+        del self._list[index]
+
+    def __iter__(self):
+        return iter(self._list)
+
+    def __eq__(self, other):
+        return self._list == other


Test patch:
diff --git a/pydicom/tests/test_dataelem.py b/pydicom/tests/test_dataelem.py
index 2fbc4709d..56d0c49d4 100644
--- a/pydicom/tests/test_dataelem.py
+++ b/pydicom/tests/test_dataelem.py
@@ -11,6 +11,8 @@ import unittest
 
 import sys
 
+from pydicom.valuerep import DSfloat
+
 from pydicom.charset import default_encoding
 
 from pydicom.dataelem import DataElement
@@ -43,6 +45,26 @@ class DataElementTests(unittest.TestCase):
         self.assertEqual(VM, 1,
                          "Wrong Value Multiplicity, expected 1, got %i" % VM)
 
+    def testDSFloatConversion(self):
+        """Test that strings are correctly converted if changing the value."""
+        self.assertTrue(isinstance(self.data_elementDS.value, DSfloat))
+        self.assertTrue(isinstance(self.data_elementMulti.value[0], DSfloat))
+        self.assertEqual(DSfloat('42.1'), self.data_elementMulti.value[0])
+
+        # multi-value append/insert
+        self.data_elementMulti.value.append('42.4')
+        self.assertTrue(isinstance(self.data_elementMulti.value[3], DSfloat))
+        self.assertEqual(DSfloat('42.4'), self.data_elementMulti.value[3])
+
+        self.data_elementMulti.value.insert(0, '42.0')
+        self.assertTrue(isinstance(self.data_elementMulti.value[0], DSfloat))
+        self.assertEqual(DSfloat('42.0'), self.data_elementMulti.value[0])
+
+        # change single value of multi-value
+        self.data_elementMulti.value[3] = '123.4'
+        self.assertTrue(isinstance(self.data_elementMulti.value[3], DSfloat))
+        self.assertEqual(DSfloat('123.4'), self.data_elementMulti.value[3])
+
     def testBackslash(self):
         """DataElement: String with '\\' sets multi-valued data_element."""
         data_element = DataElement((1, 2), "DS", r"42.1\42.2\42.3")
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index bc78943b1..777462a9e 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -274,10 +274,71 @@ class WriteDataElementTests(unittest.TestCase):
         got = self.f1.getvalue()
         msg = ("Did not write zero-length AT value correctly. "
                "Expected %r, got %r") % (bytes2hex(expected), bytes2hex(got))
-        msg = "%r %r" % (type(expected), type(got))
-        msg = "'%r' '%r'" % (expected, got)
         self.assertEqual(expected, got, msg)
 
+    def check_data_element(self, data_elem, expected):
+        encoded_elem = self.encode_element(data_elem)
+        self.assertEqual(expected, encoded_elem)
+
+    def test_write_DA(self):
+        data_elem = DataElement(0x00080022, 'DA', '20000101')
+        expected = (b'\x08\x00\x22\x00'  # tag
+                    b'\x08\x00\x00\x00'  # length
+                    b'20000101')  # value
+        self.check_data_element(data_elem, expected)
+        data_elem = DataElement(0x00080022, 'DA', date(2000, 1, 1))
+        self.check_data_element(data_elem, expected)
+
+    def test_write_multi_DA(self):
+        data_elem = DataElement(0x0014407E, 'DA', ['20100101', '20101231'])
+        expected = (b'\x14\x00\x7E\x40'  # tag
+                    b'\x12\x00\x00\x00'  # length
+                    b'20100101\\20101231 ')  # padded value
+        self.check_data_element(data_elem, expected)
+        data_elem = DataElement(0x0014407E, 'DA', [date(2010, 1, 1),
+                                                   date(2010, 12, 31)])
+        self.check_data_element(data_elem, expected)
+
+    def test_write_TM(self):
+        data_elem = DataElement(0x00080030, 'TM', '010203')
+        expected = (b'\x08\x00\x30\x00'  # tag
+                    b'\x06\x00\x00\x00'  # length
+                    b'010203')  # padded value
+        self.check_data_element(data_elem, expected)
+        data_elem = DataElement(0x00080030, 'TM', time(1, 2, 3))
+        self.check_data_element(data_elem, expected)
+
+    def test_write_multi_TM(self):
+        data_elem = DataElement(0x0014407C, 'TM', ['082500', '092655'])
+        expected = (b'\x14\x00\x7C\x40'  # tag
+                    b'\x0E\x00\x00\x00'  # length
+                    b'082500\\092655 ')  # padded value
+        self.check_data_element(data_elem, expected)
+        data_elem = DataElement(0x0014407C, 'TM', [time(8, 25),
+                                                   time(9, 26, 55)])
+        self.check_data_element(data_elem, expected)
+
+    def test_write_DT(self):
+        data_elem = DataElement(0x0008002A, 'DT', '20170101120000')
+        expected = (b'\x08\x00\x2A\x00'  # tag
+                    b'\x0E\x00\x00\x00'  # length
+                    b'20170101120000')  # value
+        self.check_data_element(data_elem, expected)
+        data_elem = DataElement(0x0008002A, 'DT', datetime(2017, 1, 1, 12))
+        self.check_data_element(data_elem, expected)
+
+    def test_write_multi_DT(self):
+        data_elem = DataElement(0x0040A13A, 'DT',
+                                ['20120820120804', '20130901111111'])
+        expected = (b'\x40\x00\x3A\xA1'  # tag
+                    b'\x1E\x00\x00\x00'  # length
+                    b'20120820120804\\20130901111111 ')  # padded value
+        self.check_data_element(data_elem, expected)
+        data_elem = DataElement(0x0040A13A, 'DT',
+                                [datetime(2012, 8, 20, 12, 8, 4),
+                                 datetime(2013, 9, 1, 11, 11, 11)])
+        self.check_data_element(data_elem, expected)
+
     def test_write_OD_implicit_little(self):
         """Test writing elements with VR of OD works correctly."""
         # VolumetricCurvePoints
diff --git a/pydicom/tests/test_multival.py b/pydicom/tests/test_multival.py
index b44ad7f65..df277e640 100644
--- a/pydicom/tests/test_multival.py
+++ b/pydicom/tests/test_multival.py
@@ -26,6 +26,18 @@ class MultiValuetests(unittest.TestCase):
             self.assertTrue(isinstance(val, (DSfloat, DSdecimal)),
                             "Multi-value DS item not converted to DS")
 
+    def testEmptyElements(self):
+        """MultiValue: Empty number string elements are not converted..."""
+        multival = MultiValue(DSfloat, ['1.0', ''])
+        self.assertEqual(1.0, multival[0])
+        self.assertEqual('', multival[1])
+        multival = MultiValue(IS, ['1', ''])
+        self.assertEqual(1, multival[0])
+        self.assertEqual('', multival[1])
+        multival = MultiValue(DSdecimal, ['1', ''])
+        self.assertEqual(1, multival[0])
+        self.assertEqual('', multival[1])
+
     def testLimits(self):
         """MultiValue: Raise error if any item outside DICOM limits...."""
         original_flag = config.enforce_valid_values
@@ -52,6 +64,14 @@ class MultiValuetests(unittest.TestCase):
         self.assertEqual(multival[1], 7,
                          "Item set by index is not correct value")
 
+    def testDeleteIndex(self):
+        """MultiValue: Deleting item at index behaves as expected..."""
+        multival = MultiValue(IS, [1, 5, 10])
+        del multival[1]
+        self.assertEqual(2, len(multival))
+        self.assertEqual(multival[0], 1)
+        self.assertEqual(multival[1], 10)
+
     def testExtend(self):
         """MultiValue: Extending a list converts all to required type"""
         multival = MultiValue(IS, [1, 5, 10])