Repository: transientskp/pyse

Issue:
Implementation of `ParamSet` is inconsistent
### Description

Output from mypy
```
sourcefinder/extract.py:244: error: Cannot assign to a method  [method-assign]
sourcefinder/extract.py:244: error: Incompatible types in assignment (expression has type "dict[str, Uncertain]", variable has type "Callable[[], ValuesView[Any]]")  [assignment]
sourcefinder/extract.py:271: error: Value of type "Callable[[], ValuesView[Any]]" is not indexable  [index]
sourcefinder/extract.py:274: error: Unsupported right operand type for in ("Callable[[], ValuesView[Any]]")  [operator]
sourcefinder/extract.py:276: error: Unsupported target for indexed assignment ("Callable[[], ValuesView[Any]]")  [index]
sourcefinder/extract.py:278: error: Value of type "Callable[[], ValuesView[Any]]" is not indexable  [index]
sourcefinder/extract.py:279: error: Unsupported right operand type for in ("Callable[[], ValuesView[Any]]")  [operator]
sourcefinder/extract.py:280: error: Value of type "Callable[[], ValuesView[Any]]" is not indexable  [index]
sourcefinder/extract.py:295: error: "Callable[[], ValuesView[Any]]" has no attribute "keys"  [attr-defined]
```

The implementation is inconsistent.  While it inherits from `collections.abc.MutableMapping`, it doesn't respect the [mapping API](https://docs.python.org/3/glossary.html#term-mapping)

Problem is here (the first error above):
https://github.com/transientskp/pyse/blob/1de96f08469410abccd9e7a973f89d963faa4c2a/sourcefinder/extract.py#L244-L255

For example, in the line mentioned in the error above, it should not be overriding the `values()` method.  AFAIU, the implementation stores it's data as a regular `dict` in `ParamSet.values`, and all downstream use looks like this: `instance.values.<dict_method call>` instead of `instance.<dict_method_call>`.

### Resolution

1. Either `MutableMapping` should be removed.

   ```diff
   @@ -220,7 +220,7 @@ class Island(object):
            return measurement, gauss_residual
    
    
   -class ParamSet(MutableMapping):
   +class ParamSet:
        """
        All the source fitting methods should go to produce a ParamSet, which
        gives all the information necessary to make a Detection.
   ```

2. Or the implementation should be fixed to reflect the Python mapping API.


Discussion:

Patch:
diff --git a/sourcefinder/extract.py b/sourcefinder/extract.py
index 1fd5bbf..cb74d86 100644
--- a/sourcefinder/extract.py
+++ b/sourcefinder/extract.py
@@ -241,7 +241,7 @@ class ParamSet(MutableMapping):
         self.alpha_maj3 = alpha_maj3
         self.alpha_min3 = alpha_min3
 
-        self.values = {
+        self.measurements = {
             'peak': Uncertain(),
             'flux': Uncertain(),
             'xbar': Uncertain(),
@@ -268,31 +268,29 @@ class ParamSet(MutableMapping):
         self.reduced_chisq = None
 
     def __getitem__(self, item):
-        return self.values[item]
+        return self.measurements[item]
 
     def __setitem__(self, item, value):
-        if item in self.values:
+        if item in self.measurements:
             if isinstance(value, Uncertain):
-                self.values[item] = value
+                self.measurements[item] = value
             else:
-                self.values[item].value = value
-        elif item[:3] == 'err' and item[3:] in self.values:
-            self.values[item[3:]].error = value
+                self.measurements[item].value = value
         else:
             raise AttributeError("Invalid parameter")
 
     def __delitem__(self, key):
-        raise Exception
+        del self.measurements[key]
 
     def __iter__(self):
-        raise Exception
+        return iter(self.measurements)
 
     def __len__(self):
-        raise Exception
+        return len(self.measurements)
 
     def keys(self):
         """ """
-        return list(self.values.keys())
+        return list(self.measurements.keys())
 
     def calculate_errors(self, noise, max_pix_variance_factor, correlation_lengths, threshold):
         """Calculate positional errors


Test patch:
diff --git a/test/test_errors.py b/test/test_errors.py
index 1446037..19ed95a 100644
--- a/test/test_errors.py
+++ b/test/test_errors.py
@@ -15,7 +15,7 @@ class DummyImage(object):
 def get_paramset():
     paramset = ParamSet()
     paramset.sig = 1
-    paramset.values = {
+    paramset.measurements = {
         'peak': Uncertain(10, 0),
         'flux': Uncertain(10, 0),
         'semimajor': Uncertain(10, 1),
@@ -100,13 +100,13 @@ class TestPositionErrors(unittest.TestCase):
         # See HipChat discussion of 2013-08-28.
         # Values of all parameters are dummies except for the pixel position.
         # First, construct a source at the NCP
-        self.p.values['xbar'] = Uncertain(1025, 1)
-        self.p.values['ybar'] = Uncertain(1025, 1)
+        self.p.measurements['xbar'] = Uncertain(1025, 1)
+        self.p.measurements['ybar'] = Uncertain(1025, 1)
         d_ncp = Detection(self.p, self.ncp_image)
 
         # Then construct a source somewhere away from the NCP
-        self.p.values['xbar'] = Uncertain(125, 1)
-        self.p.values['ybar'] = Uncertain(125, 1)
+        self.p.measurements['xbar'] = Uncertain(125, 1)
+        self.p.measurements['ybar'] = Uncertain(125, 1)
         d_not_ncp = Detection(self.p, self.ncp_image)
 
         # One source is at higher declination
@@ -118,24 +118,24 @@ class TestPositionErrors(unittest.TestCase):
     def test_error_radius_value(self):
         # Demonstrate that we calculate the expected value for the error
         # radius
-        self.p.values['xbar'] = Uncertain(1025, 1)
-        self.p.values['ybar'] = Uncertain(1025, 1)
+        self.p.measurements['xbar'] = Uncertain(1025, 1)
+        self.p.measurements['ybar'] = Uncertain(1025, 1)
         d_ncp = Detection(self.p, self.ncp_image)
 
         # cdelt gives the per-pixel increment along the axis in degrees
         # Detection.error_radius is in arcsec
         expected_error_radius = math.sqrt(
-            (self.p.values['xbar'].error * self.ncp_image.wcs.cdelt[
+            (self.p.measurements['xbar'].error * self.ncp_image.wcs.cdelt[
                 0] * 3600) ** 2 +
-            (self.p.values['ybar'].error * self.ncp_image.wcs.cdelt[
+            (self.p.measurements['ybar'].error * self.ncp_image.wcs.cdelt[
                 1] * 3600) ** 2
         )
 
         self.assertAlmostEqual(d_ncp.error_radius, expected_error_radius, 6)
 
     def test_error_radius_with_dec(self):
-        self.p.values['xbar'] = Uncertain(1025, 1)
-        self.p.values['ybar'] = Uncertain(1025, 1)
+        self.p.measurements['xbar'] = Uncertain(1025, 1)
+        self.p.measurements['ybar'] = Uncertain(1025, 1)
         d_ncp = Detection(self.p, self.ncp_image)
         d_equator = Detection(self.p, self.equator_image)
         self.assertEqual(d_ncp.error_radius, d_equator.error_radius)
diff --git a/test/test_image.py b/test/test_image.py
index e271a90..04b9abc 100644
--- a/test/test_image.py
+++ b/test/test_image.py
@@ -205,7 +205,7 @@ class TestFitFixedPositions(unittest.TestCase):
         img = self.image
 
         fake_params = sourcefinder.extract.ParamSet()
-        fake_params.values.update({
+        fake_params.measurements.update({
             'peak': Uncertain(0.0, 0.5),
             'flux': Uncertain(0.0, 0.5),
             'xbar': Uncertain(5.5, 10000.5),  # Danger Will Robinson