Repository: pydicom/pydicom

Issue:
1.4.2: Unknown value representation #1067 continued in Pydicom 2.1.2?
Hi! I am afraid I am quite new to pydicom and Github. I am trying to process a lot of Dicom images collected over many years, and I am getting this error for some of the images. I read the previous post 1.4.2: Unknown value representation #1067 and I seem to find the same error using pydicom 2.1.2. 

In this example, an exception happens for tag(0010, 0020) which says VR=LO and its value is a 7 digit alphanumerical code. However, please note that the problem is not always for the same tag.

My output:
```
ds=pydicom.dcmread("D031232_20081014_4.13.dcm")
print(ds)

Traceback (most recent call last):

  File "<ipython-input-9-6450c0e5dcdf>", line 7, in <module>
    print(ds)

  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataset.py", line 2130, in __str__
    return self._pretty_str()

  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataset.py", line 1858, in _pretty_str
    strings.append(indent_str + repr(elem))

  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\contextlib.py", line 130, in __exit__
    self.gen.throw(type, value, traceback)

  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\tag.py", line 32, in tag_in_exception
    raise type(exc)(msg) from exc

NotImplementedError: With tag (0010, 1002) got exception: Unknown Value Representation '0x0a 0x00' in tag (0010, 0020)
Traceback (most recent call last):
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataelem.py", line 793, in DataElement_from_raw
    value = convert_value(VR, raw, encoding)
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\values.py", line 710, in convert_value
    raise NotImplementedError(f"Unknown Value Representation '{VR}'")
NotImplementedError: Unknown Value Representation '0x0a 0x00'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\tag.py", line 28, in tag_in_exception
    yield
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataset.py", line 1855, in _pretty_str
    strings.append(dataset._pretty_str(indent + 1))
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataset.py", line 1846, in _pretty_str
    for elem in self:
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataset.py", line 1234, in __iter__
    yield self[tag]
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataset.py", line 935, in __getitem__
    self[tag] = DataElement_from_raw(data_elem, character_set)
  File "C:\Users\myname\AppData\Local\Continuum\anaconda3\lib\site-packages\pydicom\dataelem.py", line 795, in DataElement_from_raw
    raise NotImplementedError("{0:s} in tag {1!r}".format(str(e), raw.tag))
NotImplementedError: Unknown Value Representation '0x0a 0x00' in tag (0010, 0020)
```






Discussion:
pep8speaks: Hello @darcymason! 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_rawread.py`](https://github.com/pydicom/pydicom/blob/4cf116f432c574102de5ce67915b2fbbbcf3e44c/pydicom/tests/test_rawread.py):

> [Line 558:48](https://github.com/pydicom/pydicom/blob/4cf116f432c574102de5ce67915b2fbbbcf3e44c/pydicom/tests/test_rawread.py#L558): [W292](https://duckduckgo.com/?q=pep8%20W292) no newline at end of file


codecov[bot]: # [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=h1) Report
> Merging [#1315](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=desc) (bbf53a8) into [master](https://codecov.io/gh/pydicom/pydicom/commit/6ecfedef5a9de7fdafe931fc071648cf2c3b8542?el=desc) (6ecfede) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pydicom/pydicom/pull/1315/graphs/tree.svg?width=650&height=150&src=pr&token=YciLV15Lwr)](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1315   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files          60       60           
  Lines        9517     9524    +7     
=======================================
+ Hits         9064     9071    +7     
  Misses        453      453           
```


| [Impacted Files](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pydicom/config.py](https://codecov.io/gh/pydicom/pydicom/pull/1315/diff?src=pr&el=tree#diff-cHlkaWNvbS9jb25maWcucHk=) | `96.20% <100.00%> (+0.04%)` | :arrow_up: |
| [pydicom/filereader.py](https://codecov.io/gh/pydicom/pydicom/pull/1315/diff?src=pr&el=tree#diff-cHlkaWNvbS9maWxlcmVhZGVyLnB5) | `93.86% <100.00%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=footer). Last update [6ecfede...4cf116f](https://codecov.io/gh/pydicom/pydicom/pull/1315?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

darcymason: I've created this with a config option, but left it as default `True` -- an explicit VR read with non-character VR is pretty much doomed to fail anyway, so I don't think backwards-compatibility is a problem.

Patch:
diff --git a/doc/reference/config.rst b/doc/reference/config.rst
index ad971cc3c..f4e370bde 100644
--- a/doc/reference/config.rst
+++ b/doc/reference/config.rst
@@ -11,6 +11,7 @@ Configuration Options (:mod:`pydicom.config`)
    :toctree: generated/
 
    allow_DS_float
+   assume_implicit_vr_switch
    convert_wrong_length_to_UN
    data_element_callback
    data_element_callback_kwargs
diff --git a/pydicom/config.py b/pydicom/config.py
index 78f7253de..bf764dbcd 100644
--- a/pydicom/config.py
+++ b/pydicom/config.py
@@ -302,6 +302,19 @@ using the pixel data handlers. If ``False`` then only rely on the element
 values within the dataset when applying corrections.
 """
 
+assume_implicit_vr_switch = True
+"""If invalid VR encountered, assume file switched to implicit VR
+
+.. versionadded:: 2.2
+
+If ``True`` (default), when reading an explicit VR file,
+if a VR is encountered that is not a valid two bytes within A-Z,
+then assume the original writer switched to implicit VR.  This has been
+seen in particular in some sequences.  This does not test that
+the VR is a valid DICOM VR, just that it has valid characters.
+"""
+
+
 INVALID_KEYWORD_BEHAVIOR = "WARN"
 """Control the behavior when setting a :class:`~pydicom.dataset.Dataset`
 attribute that's not a known element keyword.
diff --git a/pydicom/filereader.py b/pydicom/filereader.py
index 7513c3a55..0e2b59715 100644
--- a/pydicom/filereader.py
+++ b/pydicom/filereader.py
@@ -100,8 +100,11 @@ def data_element_generator(fp,
         endian_chr = "<"
     else:
         endian_chr = ">"
+
+    # assign implicit VR struct to variable as use later if VR assumed missing
+    implicit_VR_struct = Struct(endian_chr + "HHL")
     if is_implicit_VR:
-        element_struct = Struct(endian_chr + "HHL")
+        element_struct = implicit_VR_struct
     else:  # Explicit VR
         # tag, VR, 2-byte length (or 0 if special VRs)
         element_struct = Struct(endian_chr + "HH2sH")
@@ -136,12 +139,27 @@ def data_element_generator(fp,
             group, elem, length = element_struct_unpack(bytes_read)
         else:  # explicit VR
             group, elem, VR, length = element_struct_unpack(bytes_read)
-            VR = VR.decode(default_encoding)
-            if VR in extra_length_VRs:
-                bytes_read = fp_read(4)
-                length = extra_length_unpack(bytes_read)[0]
-                if debugging:
-                    debug_msg += " " + bytes2hex(bytes_read)
+            # defend against switching to implicit VR, some writer do in SQ's
+            # issue 1067, issue 1035
+
+            if config.assume_implicit_vr_switch and not (b'AA' <= VR <= b'ZZ'):
+                # invalid VR, must be two cap chrs
+                message = (
+                    "Explicit VR character(s) invalid. "
+                    "Assuming data element is implicit VR "
+                    "and attempting to continue"
+                )
+                warnings.warn(message, UserWarning)
+                VR = None
+                group, elem, length = implicit_VR_struct.unpack(bytes_read)
+            else:
+                VR = VR.decode(default_encoding)
+                if VR in extra_length_VRs:
+                    bytes_read = fp_read(4)
+                    length = extra_length_unpack(bytes_read)[0]
+                    if debugging:
+                        debug_msg += " " + bytes2hex(bytes_read)
+
         if debugging:
             debug_msg = "%-47s  (%04x, %04x)" % (debug_msg, group, elem)
             if not is_implicit_VR:
@@ -188,8 +206,8 @@ def data_element_generator(fp,
                 value = (fp_read(length) if length > 0
                          else empty_value_for_VR(VR, raw=True))
                 if debugging:
-                    dotdot = "..." if length > 12 else "   "
-                    displayed_value = value[:12] if value else b''
+                    dotdot = "..." if length > 20 else "   "
+                    displayed_value = value[:20] if value else b''
                     logger_debug("%08x: %-34s %s %r %s" %
                                  (value_tell, bytes2hex(displayed_value),
                                   dotdot, displayed_value, dotdot))


Test patch:
diff --git a/pydicom/tests/test_filereader.py b/pydicom/tests/test_filereader.py
index 06b73a249..0dd37ecb9 100644
--- a/pydicom/tests/test_filereader.py
+++ b/pydicom/tests/test_filereader.py
@@ -277,6 +277,9 @@ class TestReader:
         """Test that automatic UN conversion can be switched off."""
         replace_un_with_known_vr = config.replace_un_with_known_vr
         config.replace_un_with_known_vr = True
+        assume_implicit_vr_switch = config.assume_implicit_vr_switch
+        config.assume_implicit_vr_switch = False
+
         with pytest.raises(NotImplementedError):
             ds = dcmread(get_testdata_file("bad_sequence.dcm"))
             # accessing the elements of the faulty sequence raises
@@ -286,6 +289,7 @@ class TestReader:
         ds = dcmread(get_testdata_file("bad_sequence.dcm"))
         str(ds.CTDIPhantomTypeCodeSequence)
         config.replace_un_with_known_vr = replace_un_with_known_vr
+        config.assume_implicit_vr_switch = assume_implicit_vr_switch
 
     def test_no_pixels_read(self):
         """Returns all data elements before pixels using
@@ -933,7 +937,15 @@ class TestIncorrectVR:
         ds.remove_private_tags()  # forces it to actually parse SQ
 
 
+
 class TestUnknownVR:
+    @pytest.fixture(autouse=True)
+    def restore_config_values(self):
+        orig_impl_VR_switch = config.assume_implicit_vr_switch
+        config.assume_implicit_vr_switch = False
+        yield
+        config.assume_implicit_vr_switch = orig_impl_VR_switch
+
     @pytest.mark.parametrize(
         "vr_bytes, str_output",
         [
diff --git a/pydicom/tests/test_rawread.py b/pydicom/tests/test_rawread.py
index b01cac572..170eb47ff 100644
--- a/pydicom/tests/test_rawread.py
+++ b/pydicom/tests/test_rawread.py
@@ -534,3 +534,25 @@ class TestRawSequence:
         # They will be converted when asked for. Check some:
         assert 1 == seq[0].BeamNumber
         assert 'Beam 2' == seq[1].BeamName
+
+    def test_explVR_switch_implVR_in_SQ_item(self):
+        """Raw read: Tolerate missing VR in Sequence item"""
+        # issue 1305
+        hexstr = (
+            "30 0a 00 B0"    # (300a, 00b0) Beam Sequence
+            " 53 51"         # SQ
+            " 00 00"         # reserved
+            " FF FF FF FF"   # undefined length
+            " ff fe e0 00"   # (fffe, e000) Item Tag
+            " 00 00 00 12"   # Item (dataset) Length - 18 bytes
+            " 00 10 00 20 00 00 00 0a"  # (0010, 0020) IMPL VR with length 0a
+            " 34 34 34 34 34 34 34 34 34 34"
+            " ff fe E0 dd"    # SQ delimiter
+            " 00 00 00 00"    # zero length
+        )
+        infile = BytesIO(hex2bytes(hexstr))
+        de_gen = data_element_generator(infile,
+                                        is_implicit_VR=False,
+                                        is_little_endian=False)
+        seq = next(de_gen)
+        assert seq[0].PatientID == "4444444444"