Repository: pydicom/pydicom

Issue:
Dataset.pixel_array doesn't change unless PixelData does
#### Description
Currently `ds.pixel_array` produces a numpy array that depends on element values for Rows, Columns, Samples Per Pixel, etc, however the code for `ds.pixel_array` only changes the returned array if the value for `ds.PixelData` changes. This may lead to confusion/undesirable behaviour if the values for related elements are changed after `ds.pixel_array` is called but not the underlying pixel data.

I can't think of any real use cases except maybe in an interactive session when debugging a non-conformant dataset, but I suggest we change the way `Dataset._pixel_id` is calculated so that it takes into account changes in related elements as well.


Discussion:
scaramallion: Good point, let me take a look to see what I can do
scaramallion: Exception message is back to:

> Unable to convert the pixel data: one of Pixel Data, Float Pixel Data or Double Float Pixel Data must be present in the dataset

Patch:
diff --git a/doc/release_notes/v1.4.0.rst b/doc/release_notes/v1.4.0.rst
index 8fd5e0373..9eae3372a 100644
--- a/doc/release_notes/v1.4.0.rst
+++ b/doc/release_notes/v1.4.0.rst
@@ -20,11 +20,12 @@ Fixes
   Descriptor* elements (0028,1101-1103) and (0028,3002) (:issue:`942`)
 * Fixed handling of ISO IR 159 encoding (:issue: `917`)
 * Fixed propagation of bulk data handler in Dataset.from_json (:issue:`971`)
-* Correctly handle  DICOMDIR files with records in reverse-hierarchical order
+* Correctly handle DICOMDIR files with records in reverse-hierarchical order
   (:issue:`822`)
 * *Pixel Data* encoded using JPEG2000 and decoded using the Pillow handler no
   longer returns RGB data when the (0028,0004) *Photometric Interpretation* is
   YBR_FULL or YBR_FULL_422. (:issue:`263`, :issue:`273`, :issue:`826`)
+* Avoid possible high memory usage while reading sequences (:issue:`994`)
 
 Enhancements
 ............
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index 5afaba5dd..c8e8698e0 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -34,8 +34,9 @@ from pydicom.datadict import dictionary_VR
 from pydicom.datadict import (tag_for_keyword, keyword_for_tag,
                               repeater_has_keyword)
 from pydicom.dataelem import DataElement, DataElement_from_raw, RawDataElement
-from pydicom.pixel_data_handlers.util import (convert_color_space,
-                                              reshape_pixel_array)
+from pydicom.pixel_data_handlers.util import (
+    convert_color_space, reshape_pixel_array, get_image_pixel_ids
+)
 from pydicom.tag import Tag, BaseTag, tag_in_exception
 from pydicom.uid import (ExplicitVRLittleEndian, ImplicitVRLittleEndian,
                          ExplicitVRBigEndian, PYDICOM_IMPLEMENTATION_UID)
@@ -769,13 +770,7 @@ class Dataset(dict):
         if tag is not None:  # `name` isn't a DICOM element keyword
             tag = Tag(tag)
             if tag in self._dict:  # DICOM DataElement not in the Dataset
-                data_elem = self[tag]
-                value = data_elem.value
-                if data_elem.VR == 'SQ':
-                    # let a sequence know its parent dataset, as sequence items
-                    # may need parent dataset tags to resolve ambiguous tags
-                    value.parent = self
-                return value
+                return self[tag].value
 
         # no tag or tag not contained in the dataset
         if name == '_dict':
@@ -854,6 +849,10 @@ class Dataset(dict):
         data_elem = self._dict[tag]
 
         if isinstance(data_elem, DataElement):
+            if data_elem.VR == 'SQ' and data_elem.value:
+                # let a sequence know its parent dataset, as sequence items
+                # may need parent dataset tags to resolve ambiguous tags
+                data_elem.value.parent = self
             return data_elem
         elif isinstance(data_elem, tuple):
             # If a deferred read, then go get the value now
@@ -1288,7 +1287,7 @@ class Dataset(dict):
         Raises
         ------
         ValueError
-            If `name` is not a valid handler name.
+            If `handler_name` is not a valid handler name.
         NotImplementedError
             If the given handler or any handler, if none given, is able to
             decompress pixel data with the current transfer syntax
@@ -1302,20 +1301,11 @@ class Dataset(dict):
         decompressed and any related data elements are changed accordingly.
         """
         # Check if already have converted to a NumPy array
-        # Also check if self.PixelData has changed. If so, get new NumPy array
-        keywords = ['PixelData', 'FloatPixelData', 'DoubleFloatPixelData']
-        px_keyword = [kw for kw in keywords if kw in self]
-        if len(px_keyword) != 1:
-            raise AttributeError(
-                "Unable to convert the pixel data: one of Pixel Data, Float "
-                "Pixel Data or Double Float Pixel Data must be present in "
-                "the dataset"
-            )
-
+        # Also check if pixel data has changed. If so, get new NumPy array
         already_have = True
         if not hasattr(self, "_pixel_array"):
             already_have = False
-        elif self._pixel_id != id(getattr(self, px_keyword[0])):
+        elif self._pixel_id != get_image_pixel_ids(self):
             already_have = False
 
         if already_have:
@@ -1451,9 +1441,7 @@ class Dataset(dict):
                 self._pixel_array, 'YBR_FULL', 'RGB'
             )
 
-        keywords = ['PixelData', 'FloatPixelData', 'DoubleFloatPixelData']
-        px_keyword = [kw for kw in keywords if kw in self]
-        self._pixel_id = id(getattr(self, px_keyword[0]))
+        self._pixel_id = get_image_pixel_ids(self)
 
     def decompress(self, handler_name=''):
         """Decompresses *Pixel Data* and modifies the :class:`Dataset`
diff --git a/pydicom/filewriter.py b/pydicom/filewriter.py
index a8c7982c3..9d3b6e399 100644
--- a/pydicom/filewriter.py
+++ b/pydicom/filewriter.py
@@ -70,8 +70,8 @@ def _correct_ambiguous_vr_element(elem, ds, is_little_endian):
         #   For references, see the list at
         #   https://github.com/darcymason/pydicom/pull/298
         # PixelRepresentation is usually set in the root dataset
-        while 'PixelRepresentation' not in ds and ds.parent:
-            ds = ds.parent
+        while 'PixelRepresentation' not in ds and ds.parent and ds.parent():
+            ds = ds.parent()
         # if no pixel data is present, none if these tags is used,
         # so we can just ignore a missing PixelRepresentation in this case
         if ('PixelRepresentation' not in ds and 'PixelData' not in ds or
diff --git a/pydicom/pixel_data_handlers/util.py b/pydicom/pixel_data_handlers/util.py
index a03a90dcf..ff324bada 100644
--- a/pydicom/pixel_data_handlers/util.py
+++ b/pydicom/pixel_data_handlers/util.py
@@ -750,6 +750,59 @@ def get_expected_length(ds, unit='bytes'):
     return length
 
 
+def get_image_pixel_ids(ds):
+    """Return a dict of the pixel data affecting element's :func:`id` values.
+
+    +------------------------------------------------+
+    | Element                                        |
+    +-------------+---------------------------+------+
+    | Tag         | Keyword                   | Type |
+    +=============+===========================+======+
+    | (0028,0002) | SamplesPerPixel           | 1    |
+    +-------------+---------------------------+------+
+    | (0028,0004) | PhotometricInterpretation | 1    |
+    +-------------+---------------------------+------+
+    | (0028,0006) | PlanarConfiguration       | 1C   |
+    +-------------+---------------------------+------+
+    | (0028,0008) | NumberOfFrames            | 1C   |
+    +-------------+---------------------------+------+
+    | (0028,0010) | Rows                      | 1    |
+    +-------------+---------------------------+------+
+    | (0028,0011) | Columns                   | 1    |
+    +-------------+---------------------------+------+
+    | (0028,0100) | BitsAllocated             | 1    |
+    +-------------+---------------------------+------+
+    | (0028,0103) | PixelRepresentation       | 1    |
+    +-------------+---------------------------+------+
+    | (7FE0,0008) | FloatPixelData            | 1C   |
+    +-------------+---------------------------+------+
+    | (7FE0,0009) | DoubleFloatPixelData      | 1C   |
+    +-------------+---------------------------+------+
+    | (7FE0,0010) | PixelData                 | 1C   |
+    +-------------+---------------------------+------+
+
+    Parameters
+    ----------
+    ds : Dataset
+        The :class:`~pydicom.dataset.Dataset` containing the pixel data.
+
+    Returns
+    -------
+    dict
+        A dict containing the :func:`id` values for the elements that affect
+        the pixel data.
+
+    """
+    keywords = [
+        'SamplesPerPixel', 'PhotometricInterpretation', 'PlanarConfiguration',
+        'NumberOfFrames', 'Rows', 'Columns', 'BitsAllocated',
+        'PixelRepresentation', 'FloatPixelData', 'DoubleFloatPixelData',
+        'PixelData'
+    ]
+
+    return {kw: id(getattr(ds, kw, None)) for kw in keywords}
+
+
 def pixel_dtype(ds, as_float=False):
     """Return a :class:`numpy.dtype` for the pixel data in `ds`.
 
diff --git a/pydicom/sequence.py b/pydicom/sequence.py
index 8ed09267d..481145637 100644
--- a/pydicom/sequence.py
+++ b/pydicom/sequence.py
@@ -3,6 +3,7 @@
 
 Sequence is a list of pydicom Dataset objects.
 """
+import weakref
 
 from pydicom.dataset import Dataset
 from pydicom.multival import MultiValue
@@ -57,7 +58,8 @@ class Sequence(MultiValue):
 
     @property
     def parent(self):
-        """Return the parent :class:`~pydicom.dataset.Dataset`."""
+        """Return a weak reference to the parent
+        :class:`~pydicom.dataset.Dataset`."""
         return self._parent
 
     @parent.setter
@@ -66,7 +68,7 @@ class Sequence(MultiValue):
         :class:`Sequence` items.
         """
         if value != self._parent:
-            self._parent = value
+            self._parent = weakref.ref(value)
             for item in self._list:
                 item.parent = self._parent
 


Test patch:
diff --git a/pydicom/tests/test_dataset.py b/pydicom/tests/test_dataset.py
index 7efcda5cf..19909325b 100644
--- a/pydicom/tests/test_dataset.py
+++ b/pydicom/tests/test_dataset.py
@@ -11,6 +11,7 @@ from pydicom.dataset import Dataset, FileDataset, validate_file_meta
 from pydicom import dcmread
 from pydicom.filebase import DicomBytesIO
 from pydicom.overlay_data_handlers import numpy_handler as NP_HANDLER
+from pydicom.pixel_data_handlers.util import get_image_pixel_ids
 from pydicom.sequence import Sequence
 from pydicom.tag import Tag
 from pydicom.uid import (
@@ -1142,10 +1143,10 @@ class TestDataset(object):
         # Test that _pixel_array is returned unchanged unless required
         fpath = get_testdata_files("CT_small.dcm")[0]
         ds = dcmread(fpath)
-        ds._pixel_id = id(ds.PixelData)
+        ds._pixel_id = get_image_pixel_ids(ds)
         ds._pixel_array = 'Test Value'
         ds.convert_pixel_data()
-        assert id(ds.PixelData) == ds._pixel_id
+        assert get_image_pixel_ids(ds) == ds._pixel_id
         assert 'Test Value' == ds._pixel_array
 
     def test_pixel_array_id_changed(self):
@@ -1277,17 +1278,6 @@ class TestDataset(object):
         ds2.update(ds)
         assert 'TestC' == ds2.PatientName
 
-    def test_convert_pixel_data_no_px(self):
-        """Test convert_pixel_data() with no pixel data elements."""
-        ds = Dataset()
-        msg = (
-            r"Unable to convert the pixel data: one of Pixel Data, Float "
-            r"Pixel Data or Double Float Pixel Data must be present in "
-            r"the dataset"
-        )
-        with pytest.raises(AttributeError, match=msg):
-            ds.convert_pixel_data()
-
 
 class TestDatasetElements(object):
     """Test valid assignments of data elements"""
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index 48efabb6b..8bd55d8a4 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -875,47 +875,90 @@ class TestCorrectAmbiguousVR(object):
         assert ds[0x00283002].VR == 'US'
         assert ds.LUTDescriptor == [1, 0]
 
-    def test_ambiguous_element_in_sequence_explicit(self):
-        """Test that writing a sequence with an ambiguous element
-        as explicit transfer syntax works."""
-        # regression test for #804
+    def dataset_with_modality_lut_sequence(self, pixel_repr):
         ds = Dataset()
-        ds.PixelRepresentation = 0
+        ds.PixelRepresentation = pixel_repr
         ds.ModalityLUTSequence = [Dataset()]
         ds.ModalityLUTSequence[0].LUTDescriptor = [0, 0, 16]
         ds.ModalityLUTSequence[0].LUTExplanation = None
         ds.ModalityLUTSequence[0].ModalityLUTType = 'US'  # US = unspecified
         ds.ModalityLUTSequence[0].LUTData = b'\x0000\x149a\x1f1c\xc2637'
-
         ds.is_little_endian = True
+        return ds
+
+    def test_ambiguous_element_in_sequence_explicit_using_attribute(self):
+        """Test that writing a sequence with an ambiguous element
+        as explicit transfer syntax works if accessing the tag via keyword."""
+        # regression test for #804
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=0)
         ds.is_implicit_VR = False
         fp = BytesIO()
         ds.save_as(fp, write_like_original=True)
-
         ds = dcmread(fp, force=True)
         assert 'US' == ds.ModalityLUTSequence[0][0x00283002].VR
 
-    def test_ambiguous_element_in_sequence_implicit(self):
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=1)
+        ds.is_implicit_VR = False
+        fp = BytesIO()
+        ds.save_as(fp, write_like_original=True)
+        ds = dcmread(fp, force=True)
+        assert 'SS' == ds.ModalityLUTSequence[0][0x00283002].VR
+
+    def test_ambiguous_element_in_sequence_explicit_using_index(self):
+        """Test that writing a sequence with an ambiguous element
+        as explicit transfer syntax works if accessing the tag
+        via the tag number."""
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=0)
+        ds.is_implicit_VR = False
+        fp = BytesIO()
+        ds.save_as(fp, write_like_original=True)
+        ds = dcmread(fp, force=True)
+        assert 'US' == ds[0x00283000][0][0x00283002].VR
+
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=1)
+        ds.is_implicit_VR = False
+        fp = BytesIO()
+        ds.save_as(fp, write_like_original=True)
+        ds = dcmread(fp, force=True)
+        assert 'SS' == ds[0x00283000][0][0x00283002].VR
+
+    def test_ambiguous_element_in_sequence_implicit_using_attribute(self):
         """Test that reading a sequence with an ambiguous element
-        from a file with implicit transfer syntax works."""
+        from a file with implicit transfer syntax works if accessing the
+        tag via keyword."""
         # regression test for #804
-        ds = Dataset()
-        ds.PixelRepresentation = 0
-        ds.ModalityLUTSequence = [Dataset()]
-        ds.ModalityLUTSequence[0].LUTDescriptor = [0, 0, 16]
-        ds.ModalityLUTSequence[0].LUTExplanation = None
-        ds.ModalityLUTSequence[0].ModalityLUTType = 'US'  # US = unspecified
-        ds.ModalityLUTSequence[0].LUTData = b'\x0000\x149a\x1f1c\xc2637'
-
-        ds.is_little_endian = True
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=0)
         ds.is_implicit_VR = True
         fp = BytesIO()
         ds.save_as(fp, write_like_original=True)
         ds = dcmread(fp, force=True)
-        # we first have to access the value to trigger correcting the VR
-        assert 16 == ds.ModalityLUTSequence[0].LUTDescriptor[2]
         assert 'US' == ds.ModalityLUTSequence[0][0x00283002].VR
 
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=1)
+        ds.is_implicit_VR = True
+        fp = BytesIO()
+        ds.save_as(fp, write_like_original=True)
+        ds = dcmread(fp, force=True)
+        assert 'SS' == ds.ModalityLUTSequence[0][0x00283002].VR
+
+    def test_ambiguous_element_in_sequence_implicit_using_index(self):
+        """Test that reading a sequence with an ambiguous element
+        from a file with implicit transfer syntax works if accessing the tag
+        via the tag number."""
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=0)
+        ds.is_implicit_VR = True
+        fp = BytesIO()
+        ds.save_as(fp, write_like_original=True)
+        ds = dcmread(fp, force=True)
+        assert 'US' == ds[0x00283000][0][0x00283002].VR
+
+        ds = self.dataset_with_modality_lut_sequence(pixel_repr=1)
+        ds.is_implicit_VR = True
+        fp = BytesIO()
+        ds.save_as(fp, write_like_original=True)
+        ds = dcmread(fp, force=True)
+        assert 'SS' == ds[0x00283000][0][0x00283002].VR
+
 
 class TestCorrectAmbiguousVRElement(object):
     """Test filewriter.correct_ambiguous_vr_element"""
diff --git a/pydicom/tests/test_numpy_pixel_data.py b/pydicom/tests/test_numpy_pixel_data.py
index e3658711c..b6828a947 100644
--- a/pydicom/tests/test_numpy_pixel_data.py
+++ b/pydicom/tests/test_numpy_pixel_data.py
@@ -146,6 +146,9 @@ JPEG_2K_LOSSLESS = get_testdata_files("emri_small_jpeg_2k_lossless.dcm")[0]
 JPEG_2K = get_testdata_files("JPEG2000.dcm")[0]
 # RLE Lossless
 RLE = get_testdata_files("MR_small_RLE.dcm")[0]
+# No Image Pixel module
+NO_PIXEL = get_testdata_files("rtplan.dcm")[0]
+
 
 # Transfer Syntaxes (non-retired + Explicit VR Big Endian)
 SUPPORTED_SYNTAXES = [
@@ -471,6 +474,17 @@ class TestNumpy_NumpyHandler(object):
         # Reset
         NP_HANDLER.needs_to_convert_to_RGB = orig_fn
 
+    def test_dataset_pixel_array_no_pixels(self):
+        """Test good exception message if no pixel data in dataset."""
+        ds = dcmread(NO_PIXEL)
+        msg = (
+            r"Unable to convert the pixel data: one of Pixel Data, Float "
+            r"Pixel Data or Double Float Pixel Data must be present in the "
+            r"dataset"
+        )
+        with pytest.raises(AttributeError, match=msg):
+            ds.pixel_array
+
     @pytest.mark.parametrize("fpath, data", REFERENCE_DATA_UNSUPPORTED)
     def test_can_access_unsupported_dataset(self, fpath, data):
         """Test can read and access elements in unsupported datasets."""