Repository: simpeg/simpeg
Pull request #1235 (2023-06-08 18:12:42)

conditionally allow invertible property to also be optional
<!-- 
Thanks for contributing a pull request to SimPEG!
Remember to use a personal fork of SimPEG to propose changes.

Check out the stages of a pull request at
https://docs.simpeg.xyz/content/basic/contributing.html#pull-request

Note that we are a team of volunteers and we appreciate your
patience during the review process.

Again, thanks for contributing!

Feel free to remove lines from this template that do not apply to you pull request.
-->

#### Summary
Allow invertible properties to be optional.

#### PR Checklist
* [ ] If this is a work in progress PR, set as a Draft PR
* [ ] Linted my code according to the [style guides](https://docs.simpeg.xyz/content/basic/practices.html#style).
* [ ] Added [tests](https://docs.simpeg.xyz/content/basic/practices.html#testing) to verify changes to the code.
* [ ] Added necessary documentation to any new functions/classes following the
      expect [style](https://docs.simpeg.xyz/content/basic/practices.html#documentation).
* [ ] Added relevant method tags (i.e. `GRAV`, `EM`, etc.)
* [ ] Marked as ready for review (ff this is was a draft PR), and converted 
      to a Pull Request
* [ ] Tagged ``@simpeg/simpeg-developers`` when ready for review.

#### What does this implement/fix?
Invertible properties were implicitly required to be given a value. They would throw an error if you tried to access them before setting. This adds a flag to `props.Invertible` (and `props.PhysicalProperty`) that allows us to create optional invertible properties.

<!--
Once all tests pass and the code has been reviewed and approved, it will be merged into main
-->


Patch:
diff --git a/SimPEG/props.py b/SimPEG/props.py
index c5e0e3b64..8732b012d 100644
--- a/SimPEG/props.py
+++ b/SimPEG/props.py
@@ -68,16 +68,23 @@ class PhysicalProperty:
     reciprocal = None
 
     def __init__(
-        self, short_details, mapping=None, shape=None, default=None, dtype=None
+        self,
+        short_details,
+        mapping=None,
+        shape=None,
+        default=None,
+        dtype=None,
+        optional=False,
     ):
         self.short_details = short_details
         if mapping is not None:
             mapping.prop = self
 
         self._mapping = mapping
+        self.optional = optional
 
         self.shape = shape
-        self.dtype = None
+        self.dtype = dtype
 
     @property
     def name(self):
@@ -109,6 +116,8 @@ class PhysicalProperty:
             shape_str = ""
         else:
             shape_str = f"{scope.shape} "
+        if scope.optional:
+            shape_str = f"None or {shape_str}"
         dtype_str = f" of {scope.dtype}"
         if scope.dtype is None:
             dtype_str = ""
@@ -130,7 +139,7 @@ class PhysicalProperty:
                     return 1.0 / value
             # If I don't have a mapping
             if scope.mapping is None:
-                # I done have a reciprocal, or it doesn't have a mapping
+                # I dont have a reciprocal, or it doesn't have a mapping
                 if scope.reciprocal is None:
                     return None
                 if scope.reciprocal.mapping is None:
@@ -142,14 +151,16 @@ class PhysicalProperty:
                             )
                         )
                 # Set by mapped reciprocal
-                print("returning this thing?")
                 return 1.0 / getattr(self, scope.reciprocal.name)
 
             mapping = getattr(self, scope.mapping.name, None)
             if mapping is None:
-                raise AttributeError(
-                    f"Neither a value for `{scope.name}` or mapping for `{scope.mapping.name}` has not been set."
-                )
+                if scope.optional:
+                    return None
+                else:
+                    raise AttributeError(
+                        f"Neither a value for `{scope.name}` or mapping for `{scope.mapping.name}` has not been set."
+                    )
             if self.model is None:
                 raise AttributeError(
                     f"A `model` is required for physical property {scope.name}"
@@ -238,12 +249,13 @@ class NestedModeler:
         return property(fget=fget, fset=fset, fdel=fdel, doc=doc)
 
 
-def Invertible(property_name):
+def Invertible(property_name, optional=False):
     mapping = Mapping(f"Mapping of the inversion model to {property_name}.")
 
     physical_property = PhysicalProperty(
         f"{property_name.capitalize()} physical property model.",
         mapping=mapping,
+        optional=optional,
     )
 
     property_derivative = Derivative(


Test patch:
diff --git a/tests/base/test_Props.py b/tests/base/test_Props.py
index ff78a50dc..6b718e2f4 100644
--- a/tests/base/test_Props.py
+++ b/tests/base/test_Props.py
@@ -1,6 +1,7 @@
 import unittest
 import numpy as np
 import inspect
+import pytest
 
 import discretize
 
@@ -124,137 +125,157 @@ class NestedModels(props.HasModel):
         self.nest_model = nest_model
 
 
-class TestPropMaps(unittest.TestCase):
-    def setUp(self):
-        pass
-
-    def test_basic(self):
-        expMap = maps.ExpMap(discretize.TensorMesh((3,)))
-        assert expMap.nP == 3
-
-        for Example in [SimpleExample, ShortcutExample]:
-            PM = Example(sigmaMap=expMap)
-            assert PM.sigmaMap is not None
-            assert PM.sigmaMap is expMap
-
-            # There is currently no model, so sigma, which is mapped, fails
-            self.assertRaises(AttributeError, getattr, PM, "sigma")
-
-            PM.model = np.r_[1.0, 2.0, 3.0]
-            assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
-            # PM = pickle.loads(pickle.dumps(PM))
-            # PM = maps.ExpMap.deserialize(PM.serialize())
-
-            assert np.all(
-                PM.sigmaDeriv.todense()
-                == utils.sdiag(np.exp(np.r_[1.0, 2.0, 3.0])).todense()
-            )
-
-            # If we set sigma, we should delete the mapping
-            PM.sigma = np.r_[1.0, 2.0, 3.0]
-            assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
-            # PM = pickle.loads(pickle.dumps(PM))
-            assert PM.sigmaMap is None
-            assert PM.sigmaDeriv == 0
-
-            del PM.model
-            # sigma is not changed
-            assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
-
-    def test_reciprocal(self):
-        expMap = maps.ExpMap(discretize.TensorMesh((3,)))
-
-        PM = ReciprocalMappingExample()
-
-        self.assertRaises(AttributeError, getattr, PM, "sigma")
-        PM.sigmaMap = expMap
-        PM.model = np.r_[1.0, 2.0, 3.0]
-        assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
-        assert np.all(PM.rho == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
-
-        PM.rho = np.r_[1.0, 2.0, 3.0]
-        assert PM.rhoMap is None
-        assert PM.sigmaMap is None
-        assert PM.rhoDeriv == 0
-        assert PM.sigmaDeriv == 0
-        assert np.all(PM.sigma == 1.0 / np.r_[1.0, 2.0, 3.0])
-
-        PM.sigmaMap = expMap
-        # change your mind?
-        # PM = pickle.loads(pickle.dumps(PM))
-        PM.rhoMap = expMap
-        assert PM._sigmaMap is None
-        assert len(PM.rhoMap) == 1
-        assert len(PM.sigmaMap) == 2
-        # PM = pickle.loads(pickle.dumps(PM))
-        assert np.all(PM.rho == np.exp(np.r_[1.0, 2.0, 3.0]))
-        assert np.all(PM.sigma == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
-        # PM = pickle.loads(pickle.dumps(PM))
-        assert isinstance(PM.sigmaDeriv.todense(), np.ndarray)
-
-    def test_reciprocal_no_map(self):
-        expMap = maps.ExpMap(discretize.TensorMesh((3,)))
-
-        PM = ReciprocalExample()
-        self.assertRaises(AttributeError, getattr, PM, "sigma")
-
-        PM.sigmaMap = expMap
-        # PM = pickle.loads(pickle.dumps(PM))
-        PM.model = np.r_[1.0, 2.0, 3.0]
-        assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
-        assert np.all(PM.rho == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
-
-        PM.rho = np.r_[1.0, 2.0, 3.0]
-        assert PM.sigmaMap is None
-        assert PM.sigmaDeriv == 0
-        assert np.all(PM.sigma == 1.0 / np.r_[1.0, 2.0, 3.0])
-
-        PM.sigmaMap = expMap
-        assert len(PM.sigmaMap) == 1
-        # PM = pickle.loads(pickle.dumps(PM))
-        assert np.all(PM.rho == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
-        assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
-        assert isinstance(PM.sigmaDeriv.todense(), np.ndarray)
-
-    def test_reciprocal_no_maps(self):
-        PM = ReciprocalPropExample()
-        self.assertRaises(AttributeError, getattr, PM, "sigma")
-
-        # PM = pickle.loads(pickle.dumps(PM))
-        PM.sigma = np.r_[1.0, 2.0, 3.0]
-        # PM = pickle.loads(pickle.dumps(PM))
-
-        assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
-        # PM = pickle.loads(pickle.dumps(PM))
-        assert np.all(PM.rho == 1.0 / np.r_[1.0, 2.0, 3.0])
-
-        PM.rho = np.r_[1.0, 2.0, 3.0]
-        assert np.all(PM.sigma == 1.0 / np.r_[1.0, 2.0, 3.0])
-
-    def test_reciprocal_defaults(self):
-        PM = ReciprocalPropExampleDefaults()
-        assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
-        assert np.all(PM.rho == 1.0 / np.r_[1.0, 2.0, 3.0])
-
-        rho = np.r_[2.0, 4.0, 6.0]
-        PM.rho = rho
-        assert np.all(PM.rho == rho)
-        assert np.all(PM.sigma == 1.0 / rho)
-
-    def test_multi_parameter_inversion(self):
-        """The setup of the defaults should not invalidated the
-        mappings or other defaults.
-        """
-        PM = ComplicatedInversion()
-        params = inspect.signature(ComplicatedInversion).parameters
-
-        np.testing.assert_equal(PM.Ks, params["Ks"].default)
-        np.testing.assert_equal(PM.gamma, params["gamma"].default)
-        np.testing.assert_equal(PM.A, params["A"].default)
-
-    def test_nested(self):
-        PM = NestedModels()
-        assert PM._has_nested_models is True
+class OptionalInvertible(props.HasModel):
+    sigma, sigmaMap, sigmaDeriv = props.Invertible(
+        "Electrical conductivity (S/m)", optional=True
+    )
+
+
+@pytest.mark.parametrize("example", [SimpleExample, ShortcutExample])
+def test_basic(example):
+    expMap = maps.ExpMap(discretize.TensorMesh((3,)))
+    assert expMap.nP == 3
+
+    PM = example(sigmaMap=expMap)
+    assert PM.sigmaMap is not None
+    assert PM.sigmaMap is expMap
+
+    # There is currently no model, so sigma, which is mapped, fails
+    with pytest.raises(AttributeError):
+        PM.sigma
+
+    PM.model = np.r_[1.0, 2.0, 3.0]
+    assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
+    # PM = pickle.loads(pickle.dumps(PM))
+    # PM = maps.ExpMap.deserialize(PM.serialize())
+
+    assert np.all(
+        PM.sigmaDeriv.todense() == utils.sdiag(np.exp(np.r_[1.0, 2.0, 3.0])).todense()
+    )
+
+    # If we set sigma, we should delete the mapping
+    PM.sigma = np.r_[1.0, 2.0, 3.0]
+    assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
+    # PM = pickle.loads(pickle.dumps(PM))
+    assert PM.sigmaMap is None
+    assert PM.sigmaDeriv == 0
+
+    del PM.model
+    # sigma is not changed
+    assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
+
+
+def test_reciprocal():
+    expMap = maps.ExpMap(discretize.TensorMesh((3,)))
+
+    PM = ReciprocalMappingExample()
+
+    with pytest.raises(AttributeError):
+        PM.sigma
+    PM.sigmaMap = expMap
+    PM.model = np.r_[1.0, 2.0, 3.0]
+    assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
+    assert np.all(PM.rho == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
+
+    PM.rho = np.r_[1.0, 2.0, 3.0]
+    assert PM.rhoMap is None
+    assert PM.sigmaMap is None
+    assert PM.rhoDeriv == 0
+    assert PM.sigmaDeriv == 0
+    assert np.all(PM.sigma == 1.0 / np.r_[1.0, 2.0, 3.0])
+
+    PM.sigmaMap = expMap
+    # change your mind?
+    # PM = pickle.loads(pickle.dumps(PM))
+    PM.rhoMap = expMap
+    assert PM._sigmaMap is None
+    assert len(PM.rhoMap) == 1
+    assert len(PM.sigmaMap) == 2
+    # PM = pickle.loads(pickle.dumps(PM))
+    assert np.all(PM.rho == np.exp(np.r_[1.0, 2.0, 3.0]))
+    assert np.all(PM.sigma == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
+    # PM = pickle.loads(pickle.dumps(PM))
+    assert isinstance(PM.sigmaDeriv.todense(), np.ndarray)
+
+
+def test_reciprocal_no_map():
+    expMap = maps.ExpMap(discretize.TensorMesh((3,)))
+
+    PM = ReciprocalExample()
+    with pytest.raises(AttributeError):
+        PM.sigma
+
+    PM.sigmaMap = expMap
+    # PM = pickle.loads(pickle.dumps(PM))
+    PM.model = np.r_[1.0, 2.0, 3.0]
+    assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
+    assert np.all(PM.rho == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
+
+    PM.rho = np.r_[1.0, 2.0, 3.0]
+    assert PM.sigmaMap is None
+    assert PM.sigmaDeriv == 0
+    assert np.all(PM.sigma == 1.0 / np.r_[1.0, 2.0, 3.0])
+
+    PM.sigmaMap = expMap
+    assert len(PM.sigmaMap) == 1
+    # PM = pickle.loads(pickle.dumps(PM))
+    assert np.all(PM.rho == 1.0 / np.exp(np.r_[1.0, 2.0, 3.0]))
+    assert np.all(PM.sigma == np.exp(np.r_[1.0, 2.0, 3.0]))
+    assert isinstance(PM.sigmaDeriv.todense(), np.ndarray)
+
+
+def test_reciprocal_no_maps():
+    PM = ReciprocalPropExample()
+    with pytest.raises(AttributeError):
+        PM.sigma
+
+    # PM = pickle.loads(pickle.dumps(PM))
+    PM.sigma = np.r_[1.0, 2.0, 3.0]
+    # PM = pickle.loads(pickle.dumps(PM))
+
+    assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
+    # PM = pickle.loads(pickle.dumps(PM))
+    assert np.all(PM.rho == 1.0 / np.r_[1.0, 2.0, 3.0])
+
+    PM.rho = np.r_[1.0, 2.0, 3.0]
+    assert np.all(PM.sigma == 1.0 / np.r_[1.0, 2.0, 3.0])
+
+
+def test_reciprocal_defaults():
+    PM = ReciprocalPropExampleDefaults()
+    assert np.all(PM.sigma == np.r_[1.0, 2.0, 3.0])
+    assert np.all(PM.rho == 1.0 / np.r_[1.0, 2.0, 3.0])
+
+    rho = np.r_[2.0, 4.0, 6.0]
+    PM.rho = rho
+    assert np.all(PM.rho == rho)
+    assert np.all(PM.sigma == 1.0 / rho)
+
+
+def test_multi_parameter_inversion():
+    """The setup of the defaults should not invalidated the
+    mappings or other defaults.
+    """
+    PM = ComplicatedInversion()
+    params = inspect.signature(ComplicatedInversion).parameters
+
+    np.testing.assert_equal(PM.Ks, params["Ks"].default)
+    np.testing.assert_equal(PM.gamma, params["gamma"].default)
+    np.testing.assert_equal(PM.A, params["A"].default)
+
+
+def test_nested():
+    PM = NestedModels()
+    assert PM._has_nested_models is True
+
+
+def test_optional_inverted():
+    modeler = OptionalInvertible()
+    assert modeler.sigmaMap is None
+    assert modeler.sigma is None
+
+    modeler.sigma = 10
+    assert modeler.sigma == 10
 
 
 if __name__ == "__main__":