Implementation of `ParamSet` is inconsistent

[suvayu]

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

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.

   @@ -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.

[suvayu]

all downstream use looks like this: instance.values.<dict_method call> instead of instance.<dict_method_call>

Actually, this seems to be mixed! Some parts of the code does use the instance.<dict_method_call>, and other places use instance.values.<dict_method call>.

[HannoSpreeuw]

That override seems weird indeed. Somehow we got away with it all those years. I will see what happens if I try to remove the override and fix any upcoming errors, unless @jdswinbank has a different perspective on this problem.

[jdswinbank]

I have no immediate recollection of this. I could dig into the code, but I'm not sure it would teach us much. If you can successfully fix the errors Hanno, please go for it!