Merge pull request #1763 from pybamm-team/issue-1755-quickplot

Issue 1755 quickplot

pybamm/models/base_model.py

@@ -348,6 +348,10 @@ def _find_symbols(self, typ):
     def __getitem__(self, key):
         return self.rhs[key]
 
+    @property
+    def default_quick_plot_variables(self):
+        return None
+
     def new_empty_copy(self):
         """
         Create an empty copy of the model with the same name and "parameters"

pybamm/models/full_battery_models/base_battery_model.py

@@ -400,12 +400,6 @@ def __init__(self, options=None, name="Unnamed battery model"):
         self._built = False
         self._built_fundamental_and_external = False
 
-    @property
-    def default_parameter_values(self):
-        # Default parameter values
-        # Lion parameters left as default parameter set for tests
-        return pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Marquis2019)
-
     @property
     def default_geometry(self):
         return pybamm.battery_geometry(

pybamm/models/full_battery_models/lead_acid/base_lead_acid_model.py

@@ -54,6 +54,17 @@ def default_var_pts(self):
         var = pybamm.standard_spatial_vars
         return {var.x_n: 25, var.x_s: 41, var.x_p: 34, var.y: 10, var.z: 10}
 
+    @property
+    def default_quick_plot_variables(self):
+        return [
+            "Interfacial current density [A.m-2]",
+            "Electrolyte concentration [mol.m-3]",
+            "Current [A]",
+            "Porosity",
+            "Electrolyte potential [V]",
+            "Terminal voltage [V]",
+        ]
+
     def set_soc_variables(self):
         """Set variables relating to the state of charge."""
         # State of Charge defined as function of dimensionless electrolyte concentration

pybamm/models/full_battery_models/lithium_ion/base_lithium_ion_model.py

@@ -44,6 +44,36 @@ def __init__(self, options=None, name="Unnamed lithium-ion model", build=False):
             )
         self.set_standard_output_variables()
 
+    @property
+    def default_parameter_values(self):
+        if self.half_cell:
+            return pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
+        else:
+            return pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Marquis2019)
+
+    @property
+    def default_quick_plot_variables(self):
+        if self.half_cell:
+            return [
+                "Electrolyte concentration [mol.m-3]",
+                "Positive particle surface concentration [mol.m-3]",
+                "Current [A]",
+                "Electrolyte potential [V]",
+                "Positive electrode potential [V]",
+                "Terminal voltage [V]",
+            ]
+        else:
+            return [
+                "Negative particle surface concentration [mol.m-3]",
+                "Electrolyte concentration [mol.m-3]",
+                "Positive particle surface concentration [mol.m-3]",
+                "Current [A]",
+                "Negative electrode potential [V]",
+                "Electrolyte potential [V]",
+                "Positive electrode potential [V]",
+                "Terminal voltage [V]",
+            ]
+
     def set_standard_output_variables(self):
         super().set_standard_output_variables()
 

pybamm/plotting/quick_plot.py

@@ -143,26 +143,12 @@ def __init__(
 
         # Default output variables for lead-acid and lithium-ion
         if output_variables is None:
-            if isinstance(models[0], pybamm.lithium_ion.BaseModel):
-                output_variables = [
-                    "Negative particle surface concentration [mol.m-3]",
-                    "Electrolyte concentration [mol.m-3]",
-                    "Positive particle surface concentration [mol.m-3]",
-                    "Current [A]",
-                    "Negative electrode potential [V]",
-                    "Electrolyte potential [V]",
-                    "Positive electrode potential [V]",
-                    "Terminal voltage [V]",
-                ]
-            elif isinstance(models[0], pybamm.lead_acid.BaseModel):
-                output_variables = [
-                    "Interfacial current density [A.m-2]",
-                    "Electrolyte concentration [mol.m-3]",
-                    "Current [A]",
-                    "Porosity",
-                    "Electrolyte potential [V]",
-                    "Terminal voltage [V]",
-                ]
+            output_variables = models[0].default_quick_plot_variables
+            # raise error if still None
+            if output_variables is None:
+                raise ValueError(
+                    f"No default output variables provided for {models[0].name}"
+                )
 
         self.n_rows = n_rows or int(
             len(output_variables) // np.sqrt(len(output_variables))

pybamm/solvers/base_solver.py

@@ -50,9 +50,9 @@ def __init__(
         extrap_tol=0,
         max_steps="deprecated",
     ):
-        self._method = method
-        self._rtol = rtol
-        self._atol = atol
+        self.method = method
+        self.rtol = rtol
+        self.atol = atol
         self.root_tol = root_tol
         self.root_method = root_method
         self.extrap_tol = extrap_tol
@@ -68,30 +68,6 @@ def __init__(
         self.ode_solver = False
         self.algebraic_solver = False
 
-    @property
-    def method(self):
-        return self._method
-
-    @method.setter
-    def method(self, value):
-        self._method = value
-
-    @property
-    def rtol(self):
-        return self._rtol
-
-    @rtol.setter
-    def rtol(self, value):
-        self._rtol = value
-
-    @property
-    def atol(self):
-        return self._atol
-
-    @atol.setter
-    def atol(self, value):
-        self._atol = value
-
     @property
     def root_method(self):
         return self._root_method
@@ -112,14 +88,6 @@ def root_method(self, method):
             raise pybamm.SolverError("Root method must be an algebraic solver")
         self._root_method = method
 
-    @property
-    def root_tol(self):
-        return self._root_tol
-
-    @root_tol.setter
-    def root_tol(self, tol):
-        self._root_tol = tol
-
     def copy(self):
         """Returns a copy of the solver"""
         new_solver = copy.copy(self)

pybamm/solvers/idaklu_solver.py

@@ -86,7 +86,7 @@ def set_atol_by_variable(self, variables_with_tols, model):
         """
 
         size = model.concatenated_initial_conditions.size
-        atol = self._check_atol_type(self._atol, size)
+        atol = self._check_atol_type(self.atol, size)
         for var, tol in variables_with_tols.items():
             variable = model.variables[var]
             if isinstance(variable, pybamm.StateVector):
@@ -99,7 +99,7 @@ def set_atol_by_variable(self, variables_with_tols, model):
     def set_state_vec_tol(self, atol, state_vec, tol):
         """
         A method to set the tolerances in the atol vector of a specific
-        state variable. This method modifies self._atol
+        state variable. This method modifies self.atol
 
         Parameters
         ----------
@@ -140,7 +140,9 @@ def _check_atol_type(self, atol, size):
         if atol.size != size:
             raise pybamm.SolverError(
                 """Absolute tolerances must be either a scalar or a numpy arrray
-                of the same shape as y0 ({})""".format(size)
+                of the same shape as y0 ({})""".format(
+                    size
+                )
             )
 
         return atol
@@ -178,13 +180,13 @@ def _integrate(self, model, t_eval, inputs_dict=None):
         try:
             atol = model.atol
         except AttributeError:
-            atol = self._atol
+            atol = self.atol
 
         y0 = model.y0
         if isinstance(y0, casadi.DM):
             y0 = y0.full().flatten()
 
-        rtol = self._rtol
+        rtol = self.rtol
         atol = self._check_atol_type(atol, y0.size)
 
         if model.convert_to_format == "jax":

tests/integration/test_models/test_full_battery_models/test_lithium_ion/test_dfn_half_cell.py

@@ -11,38 +11,33 @@ class TestDFN(unittest.TestCase):
     def test_basic_processing(self):
         options = {"working electrode": "positive"}
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
 
 class TestDFNWithSEI(unittest.TestCase):
     def test_well_posed_constant(self):
         options = {"working electrode": "positive", "SEI": "constant"}
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
     def test_well_posed_reaction_limited(self):
         options = {"working electrode": "positive", "SEI": "reaction limited"}
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
     def test_well_posed_solvent_diffusion_limited(self):
         options = {"working electrode": "positive", "SEI": "solvent-diffusion limited"}
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
     def test_well_posed_electron_migration_limited(self):
         options = {"working electrode": "positive", "SEI": "electron-migration limited"}
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
     def test_well_posed_interstitial_diffusion_limited(self):
@@ -51,8 +46,7 @@ def test_well_posed_interstitial_diffusion_limited(self):
             "SEI": "interstitial-diffusion limited",
         }
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
     def test_well_posed_ec_reaction_limited(self):
@@ -61,8 +55,7 @@ def test_well_posed_ec_reaction_limited(self):
             "SEI": "ec reaction limited",
         }
         model = pybamm.lithium_ion.DFN(options)
-        param = pybamm.ParameterValues(chemistry=pybamm.parameter_sets.Xu2019)
-        modeltest = tests.StandardModelTest(model, parameter_values=param)
+        modeltest = tests.StandardModelTest(model)
         modeltest.test_all(skip_output_tests=True)
 
 

tests/unit/test_models/test_full_battery_models/test_base_battery_model.py

@@ -5,7 +5,6 @@
 import pybamm
 import unittest
 import io
-import os
 from contextlib import redirect_stdout
 
 OPTIONS_DICT = {
@@ -291,23 +290,6 @@ def test_default_solver(self):
         model.algebraic = {a: a - 1}
         self.assertIsInstance(model.default_solver, pybamm.CasadiAlgebraicSolver)
 
-    def test_default_parameters(self):
-        # check parameters are read in ok
-        model = pybamm.BaseBatteryModel()
-        self.assertEqual(
-            model.default_parameter_values["Reference temperature [K]"], 298.15
-        )
-
-        # change path and try again
-
-        cwd = os.getcwd()
-        os.chdir("..")
-        model = pybamm.BaseBatteryModel()
-        self.assertEqual(
-            model.default_parameter_values["Reference temperature [K]"], 298.15
-        )
-        os.chdir(cwd)
-
     def test_timescale(self):
         model = pybamm.BaseModel()
         self.assertEqual(model.timescale.evaluate(), 1)

tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_base_lithium_ion_model.py

@@ -3,6 +3,7 @@
 #
 import pybamm
 import unittest
+import os
 
 
 class TestBaseLithiumIonModel(unittest.TestCase):
@@ -14,6 +15,23 @@ def test_incompatible_options(self):
                 {"cell geometry": "arbitrary", "thermal": "x-lumped"}
             )
 
+    def test_default_parameters(self):
+        # check parameters are read in ok
+        model = pybamm.lithium_ion.BaseModel()
+        self.assertEqual(
+            model.default_parameter_values["Reference temperature [K]"], 298.15
+        )
+
+        # change path and try again
+
+        cwd = os.getcwd()
+        os.chdir("..")
+        model = pybamm.lithium_ion.BaseModel()
+        self.assertEqual(
+            model.default_parameter_values["Reference temperature [K]"], 298.15
+        )
+        os.chdir(cwd)
+
 
 if __name__ == "__main__":
     print("Add -v for more debug output")

tests/unit/test_plotting/test_quick_plot.py

@@ -263,6 +263,15 @@ def test_simple_ode_model(self):
 
         pybamm.close_plots()
 
+    def test_plot_with_different_models(self):
+        model = pybamm.BaseModel()
+        a = pybamm.Variable("a")
+        model.rhs = {a: pybamm.Scalar(0)}
+        model.initial_conditions = {a: pybamm.Scalar(0)}
+        solution = pybamm.CasadiSolver("fast").solve(model, [0, 1])
+        with self.assertRaisesRegex(ValueError, "No default output variables"):
+            pybamm.QuickPlot(solution)
+
     def test_spm_simulation(self):
         # SPM
         model = pybamm.lithium_ion.SPM()
@@ -292,7 +301,11 @@ def test_spm_simulation(self):
     def test_loqs_spme(self):
         t_eval = np.linspace(0, 10, 2)
 
-        for model in [pybamm.lithium_ion.SPMe(), pybamm.lead_acid.LOQS()]:
+        for model in [
+            pybamm.lithium_ion.SPMe(),
+            pybamm.lead_acid.LOQS(),
+            pybamm.lithium_ion.DFN({"working electrode": "positive"}),
+        ]:
             geometry = model.default_geometry
             param = model.default_parameter_values
             param.process_model(model)