Repository: pydicom/pydicom

Issue:
"Printing" of certain dicom files fails once, but works the second time
<!-- Instructions For Filing a Bug: https://github.com/pydicom/pydicom/blob/master/CONTRIBUTING.md#filing-bugs -->

#### Description
"Printing" of certain dicom files (see [example](https://github.com/pydicom/pydicom/files/2865551/dicom_exception.zip)) fails once, but not the second time

#### Steps/Code to Reproduce
```python
from pydicom import read_file

a = read_file('...')
print(a)
# triggers exception: AttributeError: With tag (0028, 3000) got exception: Failed to resolve ambiguous VR for tag (0028, 3002): 'Dataset' object has no attribute 'PixelRepresentation'

# try same thing again...
print(a)
# just works...
```

#### Versions
Behaviour as described above at least on:
```
Linux-4.18.0-15-generic-x86_64-with-Ubuntu-18.10-cosmic
('Python', '2.7.15+ (default, Oct  2 2018, 22:12:08) \n[GCC 8.2.0]')
('numpy', '1.14.5')
('pydicom', '1.3.0.dev0')
```
and


```
('pydicom', '1.2.2')
```

Works as expected on:
```
Linux-4.18.0-15-generic-x86_64-with-Ubuntu-18.10-cosmic
('Python', '2.7.15+ (default, Oct  2 2018, 22:12:08) \n[GCC 8.2.0]')
('pydicom', '1.0.1')
```

Discussion:
pep8speaks: Hello @mrbean-bremen! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







* In the file [`pydicom/tests/test_filewriter.py`](https://github.com/pydicom/pydicom/blob/8e8e03845604d07406cda3f4dbcace5a1c659f72/pydicom/tests/test_filewriter.py):

> [Line 877:65](https://github.com/pydicom/pydicom/blob/8e8e03845604d07406cda3f4dbcace5a1c659f72/pydicom/tests/test_filewriter.py#L877): [W605](https://duckduckgo.com/?q=pep8%20W605) invalid escape sequence '\c'
> [Line 897:65](https://github.com/pydicom/pydicom/blob/8e8e03845604d07406cda3f4dbcace5a1c659f72/pydicom/tests/test_filewriter.py#L897): [W605](https://duckduckgo.com/?q=pep8%20W605) invalid escape sequence '\c'


scaramallion: I think `parent` is a better name since its consistent with tree-traversal naming, but apart from that looks good.
mrbean-bremen: Agreed. I didn't name it `parent` because of the different types (tag vs dataset), but this doesn't actually matter. 
mrbean-bremen: Ok, done. The Circle CI builds fail for some reason (`Failed to fetch http://deb.debian.org/debian/dists/jessie-updates/main/binary-amd64/Packages  404  Not Found`), but this is probably a temporary problem of the debian repo.
mrbean-bremen: I tried the [recommended workaround](https://discuss.circleci.com/t/apt-get-update-return-404/29237/9) for the circleci, but this didn't work (the latex packages [have not been found](https://circleci.com/gh/mrbean-bremen/pydicom/167)), so it may be the best to wait for the fix in circleci and ignore the failing builds until then.

Patch:
diff --git a/doc/whatsnew/v1.3.0.rst b/doc/whatsnew/v1.3.0.rst
index 28cd2b4c8..11db76fc9 100644
--- a/doc/whatsnew/v1.3.0.rst
+++ b/doc/whatsnew/v1.3.0.rst
@@ -33,3 +33,4 @@ Fixes
 * Fixed `generate_uid()` returning non-conformant UIDs when `prefix=None`
   (:issue:`788`)
 * Avoid exception if reading from empty file (:issue:`810`)
+* Correctly handle elements with ambiguous VR in sequence items (:issue:`804`)
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index c1c5fb596..ad69b9a25 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -197,6 +197,9 @@ class Dataset(object):
         self.is_little_endian = None
         self.is_implicit_VR = None
 
+        # the parent data set, if this dataset is a sequence item
+        self.parent = None
+
     def __enter__(self):
         """Method invoked on entry to a with statement."""
         return self
@@ -555,7 +558,13 @@ class Dataset(object):
             # Try the base class attribute getter (fix for issue 332)
             return super(Dataset, self).__getattribute__(name)
         else:
-            return self[tag].value
+            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
 
     @property
     def _character_set(self):
@@ -1165,6 +1174,11 @@ class Dataset(object):
                 # don't have this tag yet->create the data_element instance
                 VR = dictionary_VR(tag)
                 data_element = DataElement(tag, VR, value)
+                if VR == 'SQ':
+                    # let a sequence know its parent dataset to pass it
+                    # to its items, who may need parent dataset tags
+                    # to resolve ambiguous tags
+                    data_element.parent = self
             else:
                 # already have this data_element, just changing its value
                 data_element = self[tag]
diff --git a/pydicom/filewriter.py b/pydicom/filewriter.py
index 6151e4ffd..23ab57f09 100644
--- a/pydicom/filewriter.py
+++ b/pydicom/filewriter.py
@@ -69,6 +69,9 @@ def _correct_ambiguous_vr_element(elem, ds, is_little_endian):
         # US if PixelRepresentation value is 0x0000, else SS
         #   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
         if ds.PixelRepresentation == 0:
             elem.VR = 'US'
             byte_type = 'H'
diff --git a/pydicom/sequence.py b/pydicom/sequence.py
index 219183b28..72b41a91d 100644
--- a/pydicom/sequence.py
+++ b/pydicom/sequence.py
@@ -43,6 +43,9 @@ class Sequence(MultiValue):
         if isinstance(iterable, Dataset):
             raise TypeError('The Sequence constructor requires an iterable')
 
+        # the parent dataset
+        self._parent = None
+
         # If no inputs are provided, we create an empty Sequence
         if not iterable:
             iterable = list()
@@ -50,6 +53,24 @@ class Sequence(MultiValue):
         # validate_dataset is used as a pseudo type_constructor
         super(Sequence, self).__init__(validate_dataset, iterable)
 
+    @property
+    def parent(self):
+        """Return the parent dataset."""
+        return self._parent
+
+    @parent.setter
+    def parent(self, value):
+        """Set the parent dataset and pass it to all items."""
+        if value != self._parent:
+            self._parent = value
+            for item in self._list:
+                item.parent = self._parent
+
+    def __setitem__(self, i, val):
+        """Set the parent dataset to the new sequence item"""
+        super(Sequence, self).__setitem__(i, val)
+        val.parent = self._parent
+
     def __str__(self):
         """String description of the Sequence."""
         lines = [str(x) for x in self]


Test patch:
diff --git a/pydicom/tests/test_filewriter.py b/pydicom/tests/test_filewriter.py
index 0d33bcbeb..ea77c15ed 100644
--- a/pydicom/tests/test_filewriter.py
+++ b/pydicom/tests/test_filewriter.py
@@ -864,6 +864,47 @@ 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
+        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.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):
+        """Test that reading a sequence with an ambiguous element
+        from a file with implicit transfer syntax works."""
+        # 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.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
+
 
 class TestCorrectAmbiguousVRElement(object):
     """Test filewriter.correct_ambiguous_vr_element"""