Repository: pydicom/pydicom

Issue:
pop from dataset raises exception
#### Description
KeyError exception thrown while popping existing key from a dataset

#### Code to Reproduce
```python3
import pydicom
filepath = "filedir/filename.dcm" # path to the DICOM file
dataset = pydicom.dcmread(filepath)
key = (0x0020, 0x0013) # Instance Number
assert key in dataset # ok
dataset.pop(key) # throws KeyError
```

#### Expected Results
Normally, the code should not fail.

#### Actual Results
A KeyError is thrown while calling `dataset.pop`:
```
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/pydicom/dataset.py", line 783, in pop
    return self._dict.pop(*args, **kwargs)
KeyError: (32, 19)
```

#### Versions
Linux-4.9.0-9-amd64-x86_64-with-debian-9.9
Python 3.5.3 (default, Sep 27 2018, 17:25:39) 
[GCC 6.3.0 20170516]
pydicom 1.2.2

Discussion:
pep8speaks: Hello @mrbean-bremen! Thanks for updating 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/dataset.py`](https://github.com/pydicom/pydicom/blob/3f59de3d5521a392c883fbe4a82af3a794ed3b84/pydicom/dataset.py):

> [Line 1160:73](https://github.com/pydicom/pydicom/blob/3f59de3d5521a392c883fbe4a82af3a794ed3b84/pydicom/dataset.py#L1160): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace




mrbean-bremen: Ok, done. Not the best idea to do the editing on the smartphone, even for a small change... 

Patch:
diff --git a/doc/whatsnew/v1.3.0.rst b/doc/whatsnew/v1.3.0.rst
index 011e8d757..4f642ed99 100644
--- a/doc/whatsnew/v1.3.0.rst
+++ b/doc/whatsnew/v1.3.0.rst
@@ -29,6 +29,8 @@ Enhancements
 
 Fixes
 .....
+* Correctly handle `Dataset.pop` and `Dataset.setdefault` for tuple and
+  keyword arguments (:issue:`852`)
 * Correctly handle encoding errors when any of the encodings are invalid
   (not just the first) (:issue:`850`)
 * Do not raise while resolving an ambiguous VR dependent on
diff --git a/pydicom/dataset.py b/pydicom/dataset.py
index b91ab5cc3..c30ada8db 100644
--- a/pydicom/dataset.py
+++ b/pydicom/dataset.py
@@ -25,7 +25,7 @@ from itertools import takewhile
 import pydicom  # for dcmwrite
 import pydicom.charset
 import pydicom.config
-from pydicom import compat
+from pydicom import compat, datadict
 from pydicom._version import __version_info__
 from pydicom.charset import default_encoding, convert_encodings
 from pydicom.config import logger
@@ -447,18 +447,14 @@ class Dataset(dict):
         bool
             True if the DataElement is in the Dataset, False otherwise.
         """
-        if isinstance(name, (str, compat.text_type)):
-            tag = tag_for_keyword(name)
-        else:
-            try:
-                tag = Tag(name)
-            except Exception:
-                return False
+        try:
+            tag = Tag(name)
+        except (ValueError, OverflowError):
+            return False
         # Test against None as (0000,0000) is a possible tag
         if tag is not None:
             return tag in self._dict
-        else:
-            return name in self._dict  # will no doubt raise an exception
+        return name in self._dict  # will no doubt raise an exception
 
     def decode(self):
         """Apply character set decoding to all DataElements in the Dataset.
@@ -1089,14 +1085,98 @@ class Dataset(dict):
         """Delete all data elements."""
         self._dict.clear()
 
-    def pop(self, *args, **kwargs):
-        return self._dict.pop(*args, **kwargs)
+    def pop(self, key, *args):
+        """Emulate dictionary `pop`, but additionally support tag ID tuple
+        and DICOM keyword.
+
+        Removes the data element for `key` if it exists and returns it,
+        otherwise returns a default value if given or raises `KeyError`.
+
+        Parameters
+        ----------
+        key: int or str or 2-tuple
+            if tuple - the group and element number of the DICOM tag
+            if int - the combined group/element number
+            if str - the DICOM keyword of the tag
+
+        *args: zero or one argument
+            defines the behavior if no tag exists for `key`: if given,
+            it defines the return value, if not given, `KeyError` is raised
+
+        Returns
+        -------
+        The data element for `key` if it exists, or the default value if given.
+
+        Raises
+        ------
+        KeyError
+            If the key is not a valid tag ID or keyword.
+            If the tag does not exist and no default is given.
+        """
+        try:
+            tag = Tag(key)
+        except (ValueError, OverflowError):
+            return self._dict.pop(key, *args)
+        return self._dict.pop(tag, *args)
 
     def popitem(self):
         return self._dict.popitem()
 
-    def setdefault(self,  *args, **kwargs):
-        return self._dict.setdefault(*args, **kwargs)
+    def setdefault(self, key, default=None):
+        """Emulate dictionary `setdefault`, but additionally support
+        tag ID tuple and DICOM keyword for `key`, and data element value
+        for `default`.
+
+        .. usage:
+
+        >>> ds = Dataset()
+        >>> pname = ds.setdefault((0x0010, 0x0010), "Test")
+        >>> pname
+        (0010, 0010) Patient's Name                      PN: 'Test'
+        >>> pname.value
+        'Test'
+        >>> psex = ds.setdefault('PatientSex',
+        ...     DataElement(0x00100040, 'CS', 'F'))
+        >>> psex.value
+        'F'
+
+        Parameters
+        ----------
+        key: int or str or 2-tuple
+            if tuple - the group and element number of the DICOM tag
+            if int - the combined group/element number
+            if str - the DICOM keyword of the tag
+
+        default: DataElement or value type or None
+            The default value that is inserted and returned if no data
+            element exists for the given key.
+            If it is not of type DataElement, a DataElement is constructed
+            instead for the given tag ID and default as value. This is only
+            possible for known tags (e.g. tags found via the dictionary
+            lookup).
+
+        Returns
+        -------
+        The data element for `key` if it exists, or the default value if
+        it is a DataElement or None, or a DataElement constructed with
+        `default` as value.
+
+        Raises
+        ------
+        KeyError
+            If the key is not a valid tag ID or keyword.
+            If no tag exists for `key`, default is not a DataElement
+            and not None, and key is not a known DICOM tag.
+        """
+        if key in self:
+            return self[key]
+        if default is not None:
+            if not isinstance(default, DataElement):
+                tag = Tag(key)
+                vr = datadict.dictionary_VR(tag)
+                default = DataElement(Tag(key), vr, default)
+            self[key] = default
+        return default
 
     def convert_pixel_data(self):
         """Convert the Pixel Data to a numpy array internally.


Test patch:
diff --git a/pydicom/tests/test_dataset.py b/pydicom/tests/test_dataset.py
index c5bc064ca..932551bbf 100644
--- a/pydicom/tests/test_dataset.py
+++ b/pydicom/tests/test_dataset.py
@@ -235,12 +235,30 @@ class DatasetTests(unittest.TestCase):
 
     def test_pop(self):
         ds = self.dummy_dataset()
+        with pytest.raises(KeyError):
+            ds.pop(0x300a00b244)
         assert 'default' == ds.pop('dummy', 'default')
         elem = ds.pop(0x300a00b2)
         assert 'unit001' == elem.value
         with pytest.raises(KeyError):
             ds.pop(0x300a00b2)
 
+    def test_pop_using_tuple(self):
+        ds = self.dummy_dataset()
+        elem = ds.pop((0x300a, 0x00b2))
+        assert 'unit001' == elem.value
+        with pytest.raises(KeyError):
+            ds.pop((0x300a, 0x00b2))
+
+    def test_pop_using_keyword(self):
+        ds = self.dummy_dataset()
+        with pytest.raises(KeyError):
+            ds.pop('InvalidName')
+        elem = ds.pop('TreatmentMachineName')
+        assert 'unit001' == elem.value
+        with pytest.raises(KeyError):
+            ds.pop('TreatmentMachineName')
+
     def test_popitem(self):
         ds = self.dummy_dataset()
         elem = ds.popitem()
@@ -257,6 +275,33 @@ class DatasetTests(unittest.TestCase):
         assert elem.value == 'Test'
         assert 2 == len(ds)
 
+    def test_setdefault_tuple(self):
+        ds = self.dummy_dataset()
+        elem = ds.setdefault((0x300a, 0x00b2), 'foo')
+        assert 'unit001' == elem.value
+        elem = ds.setdefault((0x0010, 0x0010), DataElement(
+            0x00100010, 'PN', "Test"))
+        assert elem.value == 'Test'
+        assert 2 == len(ds)
+
+    def test_setdefault_use_value(self):
+        ds = self.dummy_dataset()
+        elem = ds.setdefault((0x0010, 0x0010), "Test")
+        assert elem.value == 'Test'
+        assert 2 == len(ds)
+        with pytest.raises(KeyError, match=r'Tag \(0011, 0010\) not found '
+                                           r'in DICOM dictionary'):
+            ds.setdefault((0x0011, 0x0010), "Test")
+
+    def test_setdefault_keyword(self):
+        ds = self.dummy_dataset()
+        elem = ds.setdefault('TreatmentMachineName', 'foo')
+        assert 'unit001' == elem.value
+        elem = ds.setdefault('PatientName',
+                             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()