Merge pull request #1977 from pybamm-team/simpler-model-copy

simplify model copy

Author: valentinsulzer

pybamm/discretisations/discretisation.py

@@ -182,8 +182,8 @@ def process_model(self, model, inplace=True, check_model=True):
             # since they point to the same object
             model_disc = model
         else:
-            # create an empty copy of the original model
-            model_disc = model.new_empty_copy()
+            # create a copy of the original model
+            model_disc = model.new_copy()
 
         # Keep a record of y_slices in the model
         model_disc.y_slices = self.y_slices_explicit

pybamm/expression_tree/operations/replace_symbols.py

@@ -54,8 +54,8 @@ def process_model(self, unprocessed_model, inplace=True):
             # since they point to the same object
             model = unprocessed_model
         else:
-            # create a blank model of the same class
-            model = unprocessed_model.new_empty_copy()
+            # create a copy of the model
+            model = unprocessed_model.new_copy()
 
         new_rhs = {}
         for variable, equation in unprocessed_model.rhs.items():

pybamm/models/base_model.py

@@ -5,6 +5,7 @@
 import warnings
 from collections import OrderedDict
 
+import copy
 import casadi
 import numpy as np
 
@@ -366,35 +367,21 @@ def __getitem__(self, key):
     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"
-        (convert_to_format, etc), but empty equations and variables.
-        This is usually then called by :class:`pybamm.ParameterValues`,
-        :class:`pybamm.Discretisation`, or :class:`pybamm.SymbolReplacer`.
-        """
-        new_model = self.__class__(name=self.name)
-        new_model.use_jacobian = self.use_jacobian
-        new_model.convert_to_format = self.convert_to_format
-        new_model._timescale = self.timescale
-        new_model._length_scales = self.length_scales
-
-        # Variables from discretisation
-        new_model.is_discretised = self.is_discretised
-        new_model.y_slices = self.y_slices
-        new_model.concatenated_rhs = self.concatenated_rhs
-        new_model.concatenated_algebraic = self.concatenated_algebraic
-        new_model.concatenated_initial_conditions = self.concatenated_initial_conditions
-
-        return new_model
-
     def new_copy(self):
         """
-        Creates an identical copy of the model, using the functionality of
-        :class:`pybamm.SymbolReplacer` but without performing any replacements
+        Creates a copy of the model, explicitly copying all the mutable attributes
+        to avoid issues with shared objects.
         """
-        replacer = pybamm.SymbolReplacer({})
-        return replacer.process_model(self, inplace=False)
+        new_model = copy.copy(self)
+        new_model._rhs = self.rhs.copy()
+        new_model._algebraic = self.algebraic.copy()
+        new_model._initial_conditions = self.initial_conditions.copy()
+        new_model._boundary_conditions = self.boundary_conditions.copy()
+        new_model._variables = self.variables.copy()
+        new_model._events = self.events.copy()
+        new_model.external_variables = self.external_variables.copy()
+        new_model._variables_casadi = self._variables_casadi.copy()
+        return new_model
 
     def update(self, *submodels):
         """
@@ -435,13 +422,7 @@ def set_initial_conditions_from(self, solution, inplace=True):
         if inplace is True:
             model = self
         else:
-            model = self.new_empty_copy()
-            model.rhs = self.rhs.copy()
-            model.algebraic = self.algebraic.copy()
-            model.initial_conditions = self.initial_conditions.copy()
-            model.boundary_conditions = self.boundary_conditions.copy()
-            model.variables = self.variables.copy()
-            model.events = self.events.copy()
+            model = self.new_copy()
 
         if isinstance(solution, pybamm.Solution):
             solution = solution.last_state

pybamm/models/full_battery_models/base_battery_model.py

@@ -864,15 +864,6 @@ def build_model(self):
         self._built = True
         pybamm.logger.info("Finish building {}".format(self.name))
 
-    def new_empty_copy(self):
-        """See :meth:`pybamm.BaseModel.new_empty_copy()`"""
-        new_model = self.__class__(name=self.name, options=self.options)
-        new_model.use_jacobian = self.use_jacobian
-        new_model.convert_to_format = self.convert_to_format
-        new_model._timescale = self.timescale
-        new_model._length_scales = self.length_scales
-        return new_model
-
     @property
     def summary_variables(self):
         return self._summary_variables

pybamm/models/full_battery_models/lead_acid/basic_full.py

@@ -294,6 +294,3 @@ def __init__(self, name="Basic full model"):
                 pybamm.Event("Maximum voltage", voltage - param.voltage_high_cut),
             ]
         )
-
-    def new_empty_copy(self):
-        return pybamm.BaseModel.new_empty_copy(self)

pybamm/models/full_battery_models/lithium_ion/basic_dfn.py

@@ -286,6 +286,3 @@ def __init__(self, name="Doyle-Fuller-Newman model"):
             pybamm.Event("Minimum voltage", voltage - param.voltage_low_cut),
             pybamm.Event("Maximum voltage", voltage - param.voltage_high_cut),
         ]
-
-    def new_empty_copy(self):
-        return pybamm.BaseModel.new_empty_copy(self)

pybamm/models/full_battery_models/lithium_ion/basic_spm.py

@@ -182,5 +182,3 @@ def __init__(self, name="Single Particle Model"):
             pybamm.Event("Maximum voltage", V - param.voltage_high_cut),
         ]
 
-    def new_empty_copy(self):
-        return pybamm.BaseModel.new_empty_copy(self)

pybamm/models/full_battery_models/lithium_ion/electrode_soh_half_cell.py

@@ -75,7 +75,3 @@ def __init__(self, working_electrode, name="Electrode-specific SOH model"):
     def default_solver(self):
         # Use AlgebraicSolver as CasadiAlgebraicSolver gives unnecessary warnings
         return pybamm.AlgebraicSolver()
-
-    def new_empty_copy(self):
-        new_model = ElectrodeSOHHalfCell(self.working_electrode, name=self.name)
-        return new_model

pybamm/parameters/parameter_values.py

@@ -422,8 +422,8 @@ def process_model(self, unprocessed_model, inplace=True):
             # since they point to the same object
             model = unprocessed_model
         else:
-            # create a blank model of the same class
-            model = unprocessed_model.new_empty_copy()
+            # create a copy of the model
+            model = unprocessed_model.new_copy()
 
         if (
             len(unprocessed_model.rhs) == 0

pybamm/simulation.py

@@ -515,11 +515,11 @@ def build_for_experiment(self, check_model=True):
                 unbuilt_model,
                 parameter_values,
             ) in self.op_conds_to_model_and_param.items():
-                # It's ok to modify the models in-place as they are not accessible
-                # from outside the simulation
                 model_with_set_params = parameter_values.process_model(
-                    unbuilt_model, inplace=True
+                    unbuilt_model, inplace=False
                 )
+                # It's ok to modify the model with set parameters in place as it's
+                # not returned anywhere
                 built_model = self._disc.process_model(
                     model_with_set_params, inplace=True, check_model=check_model
                 )