Repository: pybamm-team/PyBaMM

Issue:
Solver check
Add checks to solver to make sure we don't try to solve an ODE model with a DAE solver, and vice versa

Discussion:

Patch:
diff --git a/pybamm/expression_tree/concatenations.py b/pybamm/expression_tree/concatenations.py
index 52549b374..1bef73f75 100644
--- a/pybamm/expression_tree/concatenations.py
+++ b/pybamm/expression_tree/concatenations.py
@@ -269,22 +269,6 @@ class SparseStack(Concatenation):
         children = list(children)
         super().__init__(*children, name="sparse stack", check_domain=False)
 
-    def evaluate(self, t=None, y=None, known_evals=None):
-        """ See :meth:`pybamm.Symbol.evaluate()`. """
-        children = self.cached_children
-        if known_evals is not None:
-            if self.id not in known_evals:
-                children_eval = [None] * len(children)
-                for idx, child in enumerate(children):
-                    children_eval[idx], known_evals = child.evaluate(t, y, known_evals)
-                known_evals[self.id] = self._concatenation_evaluate(children_eval)
-            return known_evals[self.id], known_evals
-        else:
-            children_eval = [None] * len(children)
-            for idx, child in enumerate(children):
-                children_eval[idx] = child.evaluate(t, y)
-            return self._concatenation_evaluate(children_eval)
-
     def _concatenation_evaluate(self, children_eval):
         """ See :meth:`Concatenation.evaluate()`. """
         return vstack(children_eval)
diff --git a/pybamm/solvers/base_solver.py b/pybamm/solvers/base_solver.py
index 1cadf7db6..18b2ad446 100644
--- a/pybamm/solvers/base_solver.py
+++ b/pybamm/solvers/base_solver.py
@@ -12,13 +12,22 @@ class BaseSolver(object):
         The tolerance for the solver (default is 1e-8).
     """
 
-    def __init__(self, tol=1e-8):
+    def __init__(self, method=None, tol=1e-8):
+        self._method = method
         self._tol = tol
 
         # Solutions
         self._t = None
         self._y = None
 
+    @property
+    def method(self):
+        return self._method
+
+    @method.setter
+    def method(self, value):
+        self._method = value
+
     @property
     def tol(self):
         return self._tol
diff --git a/pybamm/solvers/dae_solver.py b/pybamm/solvers/dae_solver.py
index 1d17ff7e1..95918aaef 100644
--- a/pybamm/solvers/dae_solver.py
+++ b/pybamm/solvers/dae_solver.py
@@ -19,8 +19,8 @@ class DaeSolver(pybamm.BaseSolver):
         The tolerance for the initial-condition solver (default is 1e-8).
     """
 
-    def __init__(self, tol=1e-8, root_method="lm", root_tol=1e-6):
-        super().__init__(tol)
+    def __init__(self, method=None, tol=1e-8, root_method="lm", root_tol=1e-6):
+        super().__init__(method, tol)
         self.root_method = root_method
         self.root_tol = root_tol
 
@@ -53,17 +53,7 @@ class DaeSolver(pybamm.BaseSolver):
 
         """
         pybamm.logger.info("Start solving {}".format(model.name))
-
-        # create simplified rhs algebraic and event expressions
-        concatenated_rhs = model.concatenated_rhs
-        concatenated_algebraic = model.concatenated_algebraic
-        events = model.events
-        if model.use_simplify:
-            # set up simplification object, for re-use of dict
-            simp = pybamm.Simplification()
-            concatenated_rhs = simp.simplify(concatenated_rhs)
-            concatenated_algebraic = simp.simplify(concatenated_algebraic)
-            events = [simp.simplify(event) for event in events]
+        concatenated_rhs, concatenated_algebraic, y0, events, jac = self.set_up(model)
 
         def residuals(t, y, ydot):
             pybamm.logger.debug(
@@ -78,12 +68,6 @@ class DaeSolver(pybamm.BaseSolver):
             alg_eval = alg_eval[:, 0]
             return np.concatenate((rhs_eval - ydot[: rhs_eval.shape[0]], alg_eval))
 
-        def rhs(t, y):
-            return concatenated_rhs.evaluate(t, y, known_evals={})[0][:, 0]
-
-        def algebraic(t, y):
-            return concatenated_algebraic.evaluate(t, y, known_evals={})[0][:, 0]
-
         # Create event-dependent function to evaluate events
         def event_fun(event):
             def eval_event(t, y):
@@ -93,20 +77,8 @@ class DaeSolver(pybamm.BaseSolver):
 
         events = [event_fun(event) for event in events]
 
-        y0 = self.calculate_consistent_initial_conditions(
-            rhs, algebraic, model.concatenated_initial_conditions[:, 0]
-        )
-
-        if model.use_jacobian:
-            # Create Jacobian from simplified rhs
-            y = pybamm.StateVector(slice(0, np.size(y0)))
-            jac_rhs = concatenated_rhs.jac(y)
-            jac_algebraic = concatenated_algebraic.jac(y)
-            if model.use_simplify:
-                jac_rhs = simp.simplify(jac_rhs)
-                jac_algebraic = simp.simplify(jac_algebraic)
-
-            jac = pybamm.SparseStack(jac_rhs, jac_algebraic)
+        # Create function to evaluate jacobian
+        if jac is not None:
 
             def jacobian(t, y):
                 return jac.evaluate(t, y, known_evals={})[0]
@@ -125,6 +97,77 @@ class DaeSolver(pybamm.BaseSolver):
 
         pybamm.logger.info("Finish solving {}".format(model.name))
 
+    def set_up(self, model):
+        """Unpack model, perform checks, simplify and calculate jacobian.
+
+        Parameters
+        ----------
+        model : :class:`pybamm.BaseModel`
+            The model whose solution to calculate. Must have attributes rhs and
+            initial_conditions
+
+        Returns
+        -------
+        concatenated_rhs : :class:`pybamm.Concatenation`
+            Right-hand side of differential equations
+        concatenated_algebraic : :class:`pybamm.Concatenation`
+            Algebraic equations, which should evaluate to zero
+        y0 : :class:`numpy.array`
+            Vector of initial conditions
+        events : list of :class:`pybamm.Symbol`
+            List of events at which the model should terminate
+        jac : :class:`pybamm.SparseStack`
+            Jacobian matrix for the differential and algebraic equations
+
+        Raises
+        ------
+        :class:`pybamm.SolverError`
+            If the model contains any algebraic equations (in which case a DAE solver
+            should be used instead)
+        """
+        # create simplified rhs algebraic and event expressions
+        concatenated_rhs = model.concatenated_rhs
+        concatenated_algebraic = model.concatenated_algebraic
+        events = model.events
+        if model.use_simplify:
+            # set up simplification object, for re-use of dict
+            simp = pybamm.Simplification()
+            concatenated_rhs = simp.simplify(concatenated_rhs)
+            concatenated_algebraic = simp.simplify(concatenated_algebraic)
+            events = [simp.simplify(event) for event in events]
+
+        # Calculate consistent initial conditions for the algebraic equations
+        def rhs(t, y):
+            return concatenated_rhs.evaluate(t, y, known_evals={})[0][:, 0]
+
+        def algebraic(t, y):
+            return concatenated_algebraic.evaluate(t, y, known_evals={})[0][:, 0]
+
+        if len(model.algebraic) > 0:
+            y0 = self.calculate_consistent_initial_conditions(
+                rhs, algebraic, model.concatenated_initial_conditions[:, 0]
+            )
+        else:
+            # can use DAE solver to solve ODE model
+            y0 = model.concatenated_initial_conditions[:, 0]
+
+        # Calculate jacobian
+        if model.use_jacobian:
+            # Create Jacobian from simplified rhs
+            y = pybamm.StateVector(slice(0, np.size(y0)))
+            jac_rhs = concatenated_rhs.jac(y)
+            jac_algebraic = concatenated_algebraic.jac(y)
+            if model.use_simplify:
+                jac_rhs = simp.simplify(jac_rhs)
+                jac_algebraic = simp.simplify(jac_algebraic)
+
+            jac = pybamm.SparseStack(jac_rhs, jac_algebraic)
+
+        else:
+            jac = None
+
+        return concatenated_rhs, concatenated_algebraic, y0, events, jac
+
     def calculate_consistent_initial_conditions(self, rhs, algebraic, y0_guess):
         """
         Calculate consistent initial conditions for the algebraic equations through
diff --git a/pybamm/solvers/ode_solver.py b/pybamm/solvers/ode_solver.py
index ee11d3971..cdc36dbc2 100644
--- a/pybamm/solvers/ode_solver.py
+++ b/pybamm/solvers/ode_solver.py
@@ -14,8 +14,8 @@ class OdeSolver(pybamm.BaseSolver):
         The tolerance for the solver (default is 1e-8).
     """
 
-    def __init__(self, tol=1e-8):
-        super().__init__(tol)
+    def __init__(self, method=None, tol=1e-8):
+        super().__init__(method, tol)
 
     def solve(self, model, t_eval):
         """Calculate the solution of the model at specified times.
@@ -30,16 +30,9 @@ class OdeSolver(pybamm.BaseSolver):
 
         """
         pybamm.logger.info("Start solving {}".format(model.name))
+        concatenated_rhs, y0, events, jac_rhs = self.set_up(model)
 
-        concatenated_rhs = model.concatenated_rhs
-        events = model.events
-        if model.use_simplify:
-            # set up simplification object, for re-use of dict
-            simp = pybamm.Simplification()
-            # create simplified rhs and event expressions
-            concatenated_rhs = simp.simplify(concatenated_rhs)
-            events = [simp.simplify(event) for event in events]
-
+        # Create function to evaluate rhs
         def dydt(t, y):
             pybamm.logger.debug("Evaluating RHS for {} at t={}".format(model.name, t))
             y = y[:, np.newaxis]
@@ -55,14 +48,8 @@ class OdeSolver(pybamm.BaseSolver):
 
         events = [event_fun(event) for event in events]
 
-        y0 = model.concatenated_initial_conditions[:, 0]
-
-        if model.use_jacobian:
-            # Create Jacobian from simplified rhs
-            y = pybamm.StateVector(slice(0, np.size(y0)))
-            jac_rhs = concatenated_rhs.jac(y)
-            if model.use_simplify:
-                jac_rhs = simp.simplify(jac_rhs)
+        # Create function to evaluate jacobian
+        if jac_rhs is not None:
 
             def jacobian(t, y):
                 return jac_rhs.evaluate(t, y, known_evals={})[0]
@@ -81,6 +68,61 @@ class OdeSolver(pybamm.BaseSolver):
 
         pybamm.logger.info("Finish solving {}".format(model.name))
 
+    def set_up(self, model):
+        """Unpack model, perform checks, simplify and calculate jacobian.
+
+        Parameters
+        ----------
+        model : :class:`pybamm.BaseModel`
+            The model whose solution to calculate. Must have attributes rhs and
+            initial_conditions
+
+        Returns
+        -------
+        concatenated_rhs : :class:`pybamm.Concatenation`
+            Right-hand side of differential equations
+        y0 : :class:`numpy.array`
+            Vector of initial conditions
+        events : list of :class:`pybamm.Symbol`
+            List of events at which the model should terminate
+        jac_rhs : :class:`pybamm.SparseStack`
+            Jacobian matrix for the differential equations
+
+        Raises
+        ------
+        :class:`pybamm.SolverError`
+            If the model contains any algebraic equations (in which case a DAE solver
+            should be used instead)
+
+        """
+        if len(model.algebraic) > 0:
+            raise pybamm.SolverError(
+                """Cannot use ODE solver to solve model with DAEs"""
+            )
+
+        concatenated_rhs = model.concatenated_rhs
+        events = model.events
+        if model.use_simplify:
+            # set up simplification object, for re-use of dict
+            simp = pybamm.Simplification()
+            # create simplified rhs and event expressions
+            concatenated_rhs = simp.simplify(concatenated_rhs)
+            events = [simp.simplify(event) for event in events]
+
+        y0 = model.concatenated_initial_conditions[:, 0]
+
+        if model.use_jacobian:
+            # Create Jacobian from simplified rhs
+            y = pybamm.StateVector(slice(0, np.size(y0)))
+            jac_rhs = concatenated_rhs.jac(y)
+            if model.use_simplify:
+                jac_rhs = simp.simplify(jac_rhs)
+
+        else:
+            jac_rhs = None
+
+        return concatenated_rhs, y0, events, jac_rhs
+
     def integrate(
         self, derivs, y0, t_eval, events=None, mass_matrix=None, jacobian=None
     ):
diff --git a/pybamm/solvers/scikits_dae_solver.py b/pybamm/solvers/scikits_dae_solver.py
index c92f8c52a..3aac5ef96 100644
--- a/pybamm/solvers/scikits_dae_solver.py
+++ b/pybamm/solvers/scikits_dae_solver.py
@@ -35,16 +35,7 @@ class ScikitsDaeSolver(pybamm.DaeSolver):
         if scikits_odes_spec is None:
             raise ImportError("scikits.odes is not installed")
 
-        super().__init__(tol, root_method, root_tol)
-        self._method = method
-
-    @property
-    def method(self):
-        return self._method
-
-    @method.setter
-    def method(self, value):
-        self._method = value
+        super().__init__(method, tol, root_method, root_tol)
 
     def integrate(
         self, residuals, y0, t_eval, events=None, mass_matrix=None, jacobian=None
diff --git a/pybamm/solvers/scikits_ode_solver.py b/pybamm/solvers/scikits_ode_solver.py
index 407f41b6a..c21b6cea3 100644
--- a/pybamm/solvers/scikits_ode_solver.py
+++ b/pybamm/solvers/scikits_ode_solver.py
@@ -33,18 +33,9 @@ class ScikitsOdeSolver(pybamm.OdeSolver):
         if scikits_odes_spec is None:
             raise ImportError("scikits.odes is not installed")
 
-        super().__init__(tol)
-        self._method = method
+        super().__init__(method, tol)
         self.linsolver = linsolver
 
-    @property
-    def method(self):
-        return self._method
-
-    @method.setter
-    def method(self, value):
-        self._method = value
-
     def integrate(
         self, derivs, y0, t_eval, events=None, mass_matrix=None, jacobian=None
     ):
diff --git a/pybamm/solvers/scipy_solver.py b/pybamm/solvers/scipy_solver.py
index 78312a567..3b1493076 100644
--- a/pybamm/solvers/scipy_solver.py
+++ b/pybamm/solvers/scipy_solver.py
@@ -20,16 +20,7 @@ class ScipySolver(pybamm.OdeSolver):
     """
 
     def __init__(self, method="BDF", tol=1e-8):
-        super().__init__(tol)
-        self.method = method
-
-    @property
-    def method(self):
-        return self._method
-
-    @method.setter
-    def method(self, value):
-        self._method = value
+        super().__init__(method, tol)
 
     def integrate(
         self, derivs, y0, t_eval, events=None, mass_matrix=None, jacobian=None


Test patch:
diff --git a/tests/unit/test_solvers/test_ode_solver.py b/tests/unit/test_solvers/test_ode_solver.py
new file mode 100644
index 000000000..5e2638958
--- /dev/null
+++ b/tests/unit/test_solvers/test_ode_solver.py
@@ -0,0 +1,30 @@
+#
+# Tests for the ODE Solver class
+#
+import pybamm
+import unittest
+
+
+class TestOdeSolver(unittest.TestCase):
+    def test_wrong_solver(self):
+        # Create model
+        model = pybamm.BaseModel()
+        var = pybamm.Variable("var")
+        model.rhs = {var: var}
+        model.algebraic = {var: var - 1}
+
+        # test errors
+        solver = pybamm.OdeSolver()
+        with self.assertRaisesRegex(
+            pybamm.SolverError, "Cannot use ODE solver to solve model with DAEs"
+        ):
+            solver.solve(model, None)
+
+
+if __name__ == "__main__":
+    print("Add -v for more debug output")
+    import sys
+
+    if "-v" in sys.argv:
+        debug = True
+    unittest.main()
diff --git a/tests/unit/test_solvers/test_scikits_solvers.py b/tests/unit/test_solvers/test_scikits_solvers.py
index bfbf65432..67f1ce417 100644
--- a/tests/unit/test_solvers/test_scikits_solvers.py
+++ b/tests/unit/test_solvers/test_scikits_solvers.py
@@ -457,6 +457,7 @@ class TestScikitsSolvers(unittest.TestCase):
         model.rhs = {var1: 0.1 * var1}
         model.algebraic = {var2: 2 * var1 - var2}
         model.initial_conditions = {var1: 1, var2: 2}
+        model.use_jacobian = False
         disc = get_discretisation_for_testing()
         disc.process_model(model)
 
@@ -551,6 +552,22 @@ class TestScikitsSolvers(unittest.TestCase):
         np.testing.assert_allclose(solver.y[0], np.exp(0.1 * solver.t))
         np.testing.assert_allclose(solver.y[-1], 2 * np.exp(0.1 * solver.t))
 
+    def test_solve_ode_model_with_dae_solver(self):
+        # Create model
+        model = pybamm.BaseModel()
+        var = pybamm.Variable("var")
+        model.rhs = {var: 0.1 * var}
+        model.initial_conditions = {var: 1}
+        disc = get_discretisation_for_testing()
+        disc.process_model(model)
+
+        # Solve
+        solver = pybamm.ScikitsDaeSolver(tol=1e-8)
+        t_eval = np.linspace(0, 1, 100)
+        solver.solve(model, t_eval)
+        np.testing.assert_array_equal(solver.t, t_eval)
+        np.testing.assert_allclose(solver.y[0], np.exp(0.1 * solver.t))
+
 
 if __name__ == "__main__":
     print("Add -v for more debug output")