Repository: pydicom/pydicom

Issue:
Failing tests using Python 3.6.7 and 3.7.1
<!-- Instructions For Filing a Bug: https://github.com/pydicom/pydicom/blob/master/CONTRIBUTING.md#filing-bugs -->

#### Description
<!-- Example: Attribute Error thrown when printing (0x0010, 0x0020) patient Id> 0-->
Some of the Python 3.6 and 3.7 [builds](https://travis-ci.org/pydicom/pydicom/builds/440145299) are consistently failing. I restarted the latest master build, and it behaves the same way.
EDIT: This happens with the latest Python builds 3.6.7 and 3.7.1, both under Linux and Windows.

#### Steps/Code to Reproduce
<!--
Example:
```py
from io import BytesIO
from pydicom import dcmread

bytestream = b'\x02\x00\x02\x00\x55\x49\x16\x00\x31\x2e\x32\x2e\x38\x34\x30\x2e\x31' \
             b'\x30\x30\x30\x38\x2e\x35\x2e\x31\x2e\x31\x2e\x39\x00\x02\x00\x10\x00' \
             b'\x55\x49\x12\x00\x31\x2e\x32\x2e\x38\x34\x30\x2e\x31\x30\x30\x30\x38' \
             b'\x2e\x31\x2e\x32\x00\x20\x20\x10\x00\x02\x00\x00\x00\x01\x00\x20\x20' \
             b'\x20\x00\x06\x00\x00\x00\x4e\x4f\x52\x4d\x41\x4c'

fp = BytesIO(bytestream)
ds = dcmread(fp, force=True)

print(ds.PatientID)
```
If the code is too long, feel free to put it in a public gist and link
it in the issue: https://gist.github.com

When possible use pydicom testing examples to reproduce the errors. Otherwise, provide
an anonymous version of the data in order to replicate the errors.
-->

#### Expected Results
<!-- Please paste or describe the expected results.
Example: No error is thrown and the name of the patient is printed.-->

#### Actual Results
<!-- Please paste or specifically describe the actual output or traceback.
(Use %xmode to deactivate ipython's trace beautifier)
Example: ```AttributeError: 'FileDataset' object has no attribute 'PatientID'```
-->

#### Versions
<!--
Please run the following snippet and paste the output below.
import platform; print(platform.platform())
import sys; print("Python", sys.version)
import pydicom; print("pydicom", pydicom.__version__)
-->


<!-- Thanks for contributing! -->


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

 - There are no PEP8 issues in the file [`pydicom/dataset.py`](https://github.com/mrbean-bremen/pydicom/blob/797dc3ca82267a2fc31a62f1db3596c8a7316121/pydicom/dataset.py) !

 - There are no PEP8 issues in the file [`pydicom/filereader.py`](https://github.com/mrbean-bremen/pydicom/blob/797dc3ca82267a2fc31a62f1db3596c8a7316121/pydicom/filereader.py) !


darcymason: Sorry about the delay -- just getting back from a couple of weeks away and catching up with other things.  I'll try to look at this more thoroughly in the next couple of days.

Meanwhile, has anyone looked at timing tests?  The changes are pretty minor, but sometimes these things can surprise.  

Also, were all `dict` methods already overridden by Dataset?  I think that was added some time ago, but we would have to double-check in case user code has a case that falls back to a `dict` method.
mrbean-bremen: > Also, were all dict methods already overridden by Dataset?

Actually, there are some that I haven't tested (like `pop` and `update`), I will have another look at these. It is probably better to derive from `MutableMapping` - this shall provide these methods. Also, the initialization is not correct if initializing with an initializer via `kwargs`. I did the changes in a bit of a rash - never a good idea...
mrbean-bremen: Ok, I was wrong on both counts in my last comment. 
Initialization via `kwargs` has not been supported before, so that should be ok as it is now.
Deriving from `MutableMapping` does not work, because that assumes that `__iter__` returns the keys, like in `dict`, instead of the values, as in `Dataset`. To be upwards compatible, we have to re-implement the missing methods ourselves, that would have been provided by `MutableMapping` otherwise.
mrbean-bremen: Perfomance-wise, I didn't see any significant changes (reading and writing datasets, measured using timeit) in comparison with master.
mrbean-bremen: Good point! Will change this...

Patch:
diff --git a/doc/base_element.rst b/doc/base_element.rst
index 8ae4f736f..b8b4fb426 100644
--- a/doc/base_element.rst
+++ b/doc/base_element.rst
@@ -8,12 +8,18 @@ Dataset
 
 .. currentmodule:: pydicom
 
-:class:`dataset.Dataset` is the main object you will work with
-directly. Dataset is derived from Python's ``dict``, so it inherits (and
-overrides some of) the methods of ``dict``. In other words, it is a collection
-of key:value pairs, where the key is the DICOM (group,element) tag (as a
-Tag object, described below), and the value is a DataElement instance (also
-described below).
+:class:`dataset.Dataset` is the main object you will work with directly.
+Dataset wraps a dictionary, where the key is the DICOM (group,element)
+tag (as a Tag object, described below), and the value is a DataElement instance
+(also described below). It implements most of the methods of ``dict``, so
+that it mostly behaves like the wrapped ``dict``. This allows direct access
+to the data elements via the the tags, as shown below.
+
+.. note::
+
+  The iterator of a ``DataSet`` yields ``DataElement`` values, e.g. the
+  values of the dictionary, as opposed to the keys yielded by a ``dict``
+  iterator.
 
 A dataset could be created directly, but you will usually get one by reading
 an existing DICOM file::
@@ -73,7 +79,7 @@ usage example of data elements removal and assignation.
 
    To understand using :class:`sequence.Sequences` in pydicom, please refer to
    this object model:
-   :class:`dataset.Dataset` (derived from Python's ``dict``)
+   :class:`dataset.Dataset` (wraps a Python ``dict``)
        ---> contains DataElement instances
           --> the value of the data element can be one of:
 
diff --git a/doc/whatsnew/v1.3.0.rst b/doc/whatsnew/v1.3.0.rst
index c44349c78..029b08933 100644
--- a/doc/whatsnew/v1.3.0.rst
+++ b/doc/whatsnew/v1.3.0.rst
@@ -14,3 +14,6 @@ Enhancements
 
 Fixes
 .....
+
+* Do not derive `Dataset` from `dict` - fixes behavior in newer Python versions
+  (:issue:`765`)
\ No newline at end of file
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index 06cd2a8a0..d1aba2db3 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -19,7 +19,6 @@ import inspect  # for __dir__
 import io
 import os
 import os.path
-import sys
 from bisect import bisect_left
 from itertools import takewhile
 
@@ -58,8 +57,9 @@ class PropertyError(Exception):
     pass
 
 
-class Dataset(dict):
-    """A collection (dictionary) of DICOM DataElements.
+class Dataset(object):
+    """Contains a collection (dictionary) of DICOM DataElements.
+    Behaves like a dictionary.
 
     Examples
     --------
@@ -102,6 +102,8 @@ class Dataset(dict):
     Retrieving DataElements:
 
     >>> elem = ds[0x00100010]
+    >>> elem
+    (0010, 0010) Patient's Name                      PN: 'CITIZEN^Joan'
     >>> elem = ds.data_element('PatientName')
     >>> elem
     (0010, 0010) Patient's Name                      PN: 'CITIZEN^Joan'
@@ -174,7 +176,12 @@ class Dataset(dict):
     def __init__(self, *args, **kwargs):
         """Create a new Dataset instance."""
         self._parent_encoding = kwargs.get('parent_encoding', default_encoding)
-        dict.__init__(self, *args)
+        if not args:
+            self.tags = {}
+        elif isinstance(args[0], Dataset):
+            self.tags = args[0].tags
+        else:
+            self.tags = args[0]
         self.is_decompressed = False
 
         # the following read_XXX attributes are used internally to store
@@ -232,7 +239,7 @@ class Dataset(dict):
         """
         data_element = DataElement(tag, VR, value)
         # use data_element.tag since DataElement verified it
-        self[data_element.tag] = data_element
+        self.tags[data_element.tag] = data_element
 
     def data_element(self, name):
         """Return the DataElement corresponding to the element keyword `name`.
@@ -255,7 +262,7 @@ class Dataset(dict):
         return None
 
     def __contains__(self, name):
-        """Extend dict.__contains__() to handle DICOM keywords.
+        """Simulate dict.__contains__() to handle DICOM keywords.
 
         This is called for code like:
         >>> 'SliceLocation' in ds
@@ -280,10 +287,9 @@ class Dataset(dict):
                 return False
         # Test against None as (0000,0000) is a possible tag
         if tag is not None:
-            return dict.__contains__(self, tag)
+            return tag in self.tags
         else:
-            return dict.__contains__(self,
-                                     name)  # will no doubt raise an exception
+            return name in self.tags  # will no doubt raise an exception
 
     def decode(self):
         """Apply character set decoding to all DataElements in the Dataset.
@@ -327,9 +333,8 @@ class Dataset(dict):
         """
         # First check if a valid DICOM keyword and if we have that data element
         tag = tag_for_keyword(name)
-        if tag is not None and tag in self:
-            dict.__delitem__(self,
-                             tag)  # direct to dict as we know we have key
+        if tag is not None and tag in self.tags:
+            del self.tags[tag]
         # If not a DICOM name in this dataset, check for regular instance name
         #   can't do delete directly, that will call __delattr__ again
         elif name in self.__dict__:
@@ -369,15 +374,15 @@ class Dataset(dict):
         # If passed a slice, delete the corresponding DataElements
         if isinstance(key, slice):
             for tag in self._slice_dataset(key.start, key.stop, key.step):
-                del self[tag]
+                del self.tags[tag]
         else:
             # Assume is a standard tag (for speed in common case)
             try:
-                dict.__delitem__(self, key)
+                del self.tags[key]
             # If not a standard tag, than convert to Tag and try again
             except KeyError:
                 tag = Tag(key)
-                dict.__delitem__(self, tag)
+                del self.tags[tag]
 
     def __dir__(self):
         """Give a list of attributes available in the Dataset.
@@ -414,7 +419,7 @@ class Dataset(dict):
             The matching DataElement keywords in the dataset. If no filters are
             used then all DataElement keywords are returned.
         """
-        allnames = [keyword_for_tag(tag) for tag in self.keys()]
+        allnames = [keyword_for_tag(tag) for tag in self.tags.keys()]
         # remove blanks - tags without valid names (e.g. private tags)
         allnames = [x for x in allnames if x]
         # Store found names in a dict, so duplicate names appear only once
@@ -449,14 +454,14 @@ class Dataset(dict):
             # Convert values() to a list for compatibility between
             #   python 2 and 3
             # Sort values() by element tag
-            self_elem = sorted(list(self.values()), key=lambda x: x.tag)
-            other_elem = sorted(list(other.values()), key=lambda x: x.tag)
+            self_elem = sorted(list(self.tags.values()), key=lambda x: x.tag)
+            other_elem = sorted(list(other.tags.values()), key=lambda x: x.tag)
             return self_elem == other_elem
 
         return NotImplemented
 
     def get(self, key, default=None):
-        """Extend dict.get() to handle DICOM DataElement keywords.
+        """Simulate dict.get() to handle DICOM DataElement tags and keywords.
 
         Parameters
         ----------
@@ -491,10 +496,17 @@ class Dataset(dict):
                 except Exception:
                     raise TypeError("Dataset.get key must be a string or tag")
         try:
-            return_val = self.__getitem__(key)
+            return self.__getitem__(key)
         except KeyError:
-            return_val = default
-        return return_val
+            return default
+
+    def keys(self):
+        """Return the DICOM tag keys to simulate dict."""
+        return self.tags.keys()
+
+    def values(self):
+        """Return the DICOM tag values to simulate dict."""
+        return self.tags.values()
 
     def __getattr__(self, name):
         """Intercept requests for Dataset attribute names.
@@ -519,7 +531,7 @@ class Dataset(dict):
             # Try the base class attribute getter (fix for issue 332)
             return super(Dataset, self).__getattribute__(name)
         tag = Tag(tag)
-        if tag not in self:  # DICOM DataElement not in the Dataset
+        if tag not in self.tags:  # DICOM DataElement not in the Dataset
             # Try the base class attribute getter (fix for issue 332)
             return super(Dataset, self).__getattribute__(name)
         else:
@@ -585,7 +597,7 @@ class Dataset(dict):
             tag = key
         else:
             tag = Tag(key)
-        data_elem = dict.__getitem__(self, tag)
+        data_elem = self.tags[tag]
 
         if isinstance(data_elem, DataElement):
             return data_elem
@@ -610,7 +622,7 @@ class Dataset(dict):
                 self[tag] = correct_ambiguous_vr_element(
                     self[tag], self, data_elem[6])
 
-        return dict.__getitem__(self, tag)
+        return self.tags.get(tag)
 
     def get_item(self, key):
         """Return the raw data element if possible.
@@ -637,7 +649,7 @@ class Dataset(dict):
             tag = key
         else:
             tag = Tag(key)
-        data_elem = dict.__getitem__(self, tag)
+        data_elem = self.tags.get(tag)
         # If a deferred read, return using __getitem__ to read and convert it
         if isinstance(data_elem, tuple) and data_elem.value is None:
             return self[key]
@@ -714,7 +726,7 @@ class Dataset(dict):
         # Note this is different than the underlying dict class,
         #        which returns the key of the key:value mapping.
         #   Here the value is returned (but data_element.tag has the key)
-        taglist = sorted(self.keys())
+        taglist = sorted(self.tags.keys())
         for tag in taglist:
             yield self[tag]
 
@@ -732,14 +744,30 @@ class Dataset(dict):
         pydicom.dataelem.DataElement or pydicom.dataelem.RawDataElement
             The Dataset's DataElements, sorted by increasing tag order.
         """
-        taglist = sorted(self.keys())
+        taglist = sorted(self.tags.keys())
         for tag in taglist:
             yield self.get_item(tag)
 
+    def __len__(self):
+        return len(self.tags)
+
     def __ne__(self, other):
         """Compare `self` and `other` for inequality."""
         return not self == other
 
+    def clear(self):
+        """Delete all data elements."""
+        self.tags.clear()
+
+    def pop(self, *args, **kwargs):
+        return self.tags.pop(*args, **kwargs)
+
+    def popitem(self):
+        return self.tags.popitem()
+
+    def setdefault(self,  *args, **kwargs):
+        return self.tags.setdefault(*args, **kwargs)
+
     def convert_pixel_data(self):
         """Convert the Pixel Data to a numpy array internally.
 
@@ -957,7 +985,7 @@ class Dataset(dict):
 
         Parameters
         ----------
-        index : int
+        indent : int
             The indent level offset (default 0)
         top_level_only : bool
             When True, only create a string for the top level elements, i.e.
@@ -1179,7 +1207,7 @@ class Dataset(dict):
                     data_element = DataElement_from_raw(
                         data_element, self._character_set)
                 data_element.private_creator = self[private_creator_tag].value
-        dict.__setitem__(self, tag, data_element)
+        self.tags[tag] = data_element
 
     def _slice_dataset(self, start, stop, step):
         """Return the element tags in the Dataset that match the slice.
@@ -1206,7 +1234,7 @@ class Dataset(dict):
         if stop is not None:
             stop = Tag(stop)
 
-        all_tags = sorted(self.keys())
+        all_tags = sorted(self.tags.keys())
         # If the Dataset is empty, return an empty list
         if not all_tags:
             return []
@@ -1292,7 +1320,7 @@ class Dataset(dict):
         recursive : bool
             Flag to indicate whether to recurse into Sequences.
         """
-        taglist = sorted(self.keys())
+        taglist = sorted(self.tags.keys())
         for tag in taglist:
 
             with tag_in_exception(tag):
diff --git a/pydicom/filereader.py b/pydicom/filereader.py
index 037680f16..9e138524f 100644
--- a/pydicom/filereader.py
+++ b/pydicom/filereader.py
@@ -540,7 +540,7 @@ def _read_file_meta_info(fp):
     start_file_meta = fp.tell()
     file_meta = read_dataset(fp, is_implicit_VR=False, is_little_endian=True,
                              stop_when=_not_group_0002)
-    if not file_meta:
+    if not file_meta.tags:
         return file_meta
 
     # Test the file meta for correct interpretation by requesting the first
@@ -760,7 +760,7 @@ def read_partial(fileobj, stop_when=None, defer_size=None,
         pass  # error already logged in read_dataset
 
     # Add the command set elements to the dataset (if any)
-    dataset.update(command_set)
+    dataset.update(command_set.tags)
 
     class_uid = file_meta_dataset.get("MediaStorageSOPClassUID", None)
     if class_uid and class_uid.name == "Media Storage Directory Storage":


Test patch:
diff --git a/pydicom/tests/test_dataset.py b/pydicom/tests/test_dataset.py
index 00dab6553..6b0a69578 100644
--- a/pydicom/tests/test_dataset.py
+++ b/pydicom/tests/test_dataset.py
@@ -180,13 +180,42 @@ class DatasetTests(unittest.TestCase):
         # Random non-existent property
         assert 'random name' not in ds
 
+    def test_clear(self):
+        ds = self.dummy_dataset()
+        assert 1 == len(ds)
+        ds.clear()
+        assert 0 == len(ds)
+
+    def test_pop(self):
+        ds = self.dummy_dataset()
+        assert 'default' == ds.pop('dummy', 'default')
+        elem = ds.pop(0x300a00b2)
+        assert 'unit001' == elem.value
+        with pytest.raises(KeyError):
+            ds.pop(0x300a00b2)
+
+    def test_popitem(self):
+        ds = self.dummy_dataset()
+        elem = ds.popitem()
+        assert 0x300a00b2 == elem[0]
+        assert 'unit001' == elem[1].value
+        with pytest.raises(KeyError):
+            ds.popitem()
+
+    def test_setdefault(self):
+        ds = self.dummy_dataset()
+        elem = ds.setdefault(0x300a00b2, 'foo')
+        assert 'unit001' == elem.value
+        elem = ds.setdefault(0x00100010, DataElement(0x00100010, 'PN', "Test"))
+        assert elem.value == 'Test'
+        assert 2 == len(ds)
+
     def testGetExists1(self):
         """Dataset: dataset.get() returns an existing item by name.........."""
         ds = self.dummy_dataset()
         unit = ds.get('TreatmentMachineName', None)
         self.assertEqual(
-            unit,
-            'unit001',
+            'unit001', unit,
             "dataset.get() did not return existing member by name")
 
     def testGetExists2(self):