Repository: tinosulzer/PyBaMM

Issue:
Unpack a concatenation of variables (overwrite Symbol.__copy__ to remove parent)
## Problem

In some cases, we want to unpack a concatenation (usually of variables). The natural way would be 
```python3
# make concatenation
var_n = Variable("var", ["negative electrode"])
var_s = Variable("var", ["separator"])
var_p = Variable("var", ["positive electrode"])
var = Concatenation(var_n, var_s, var_p)
# unpack
newvar_n, newvar_s, newvar_p = var.children
```
However, manipulating the `newvar` objects then causes issues with the expression tree (tree internal data is corrupt)

## Suggested change

Write an `unpack_concatenation` function that unpacks a Concatenation object by creating new `Variable` objects with the same `name` and `domain` but no `parent` (and no `children` - `Variable` never has children anyway)


Discussion:

Patch:
diff --git a/pybamm/discretisations/base_discretisation.py b/pybamm/discretisations/base_discretisation.py
index fb66ed60..e5326af3 100644
--- a/pybamm/discretisations/base_discretisation.py
+++ b/pybamm/discretisations/base_discretisation.py
@@ -5,6 +5,7 @@ from __future__ import absolute_import, division
 from __future__ import print_function, unicode_literals
 import pybamm
 
+import copy
 import numpy as np
 
 
@@ -262,22 +263,10 @@ class BaseDiscretisation(object):
 
             return new_symbol
 
-        elif isinstance(symbol, pybamm.Scalar):
-            return pybamm.Scalar(symbol.value, domain=symbol.domain)
-
-        elif isinstance(symbol, pybamm.Array):
-            return symbol.__class__(symbol.entries, domain=symbol.domain)
-
         else:
-            raise NotImplementedError
-            # hack to copy the symbol but without a parent
-            # (building tree from bottom up)
-            # simply setting new_symbol.parent = None, after copying, raises a TreeError
-            # parent = symbol.parent
-            # symbol.parent = None
-            # new_symbol = copy.copy(symbol)
-            # symbol.parent = parent
-            # return new_symbol
+            new_symbol = copy.deepcopy(symbol)
+            new_symbol.parent = None
+            return new_symbol
 
     def process_binary_operators(self, bin_op, y_slices, boundary_conditions):
         """Discretise binary operators in model equations.  Performs appropriate
diff --git a/pybamm/expression_tree/symbol.py b/pybamm/expression_tree/symbol.py
index d88662bb..78d4ef01 100644
--- a/pybamm/expression_tree/symbol.py
+++ b/pybamm/expression_tree/symbol.py
@@ -36,7 +36,6 @@ class Symbol(anytree.NodeMixin):
             # copy child before adding
             # this also adds copy.copy(child) to self.children
             copy.copy(child).parent = self
-
         self.domain = domain
 
     @property
@@ -101,6 +100,19 @@ class Symbol(anytree.NodeMixin):
             + tuple(self.domain)
         )
 
+    @property
+    def orphans(self):
+        """
+        Returning deepcopies of the children, with parents removed to avoid corrupting
+        the expression tree internal data
+        """
+        orp = []
+        for child in self.children:
+            new_child = copy.deepcopy(child)
+            new_child.parent = None
+            orp.append(new_child)
+        return tuple(orp)
+
     def render(self):
         """print out a visual representation of the tree (this node and its
         children)
diff --git a/pybamm/parameters/parameter_values.py b/pybamm/parameters/parameter_values.py
index 5a9535f4..eeb60a94 100644
--- a/pybamm/parameters/parameter_values.py
+++ b/pybamm/parameters/parameter_values.py
@@ -168,11 +168,6 @@ class ParameterValues(dict):
             return pybamm.Concatenation(*new_children)
 
         else:
-            # hack to copy the symbol but without a parent
-            # (building tree from bottom up)
-            # simply setting new_symbol.parent = None, after copying, raises a TreeError
-            parent = symbol.parent
-            symbol.parent = None
-            new_symbol = copy.copy(symbol)
-            symbol.parent = parent
+            new_symbol = copy.deepcopy(symbol)
+            new_symbol.parent = None
             return new_symbol


Test patch:
diff --git a/tests/test_discretisations/test_base_discretisations.py b/tests/test_discretisations/test_base_discretisations.py
index d24f0c94..e23d5bc3 100644
--- a/tests/test_discretisations/test_base_discretisations.py
+++ b/tests/test_discretisations/test_base_discretisations.py
@@ -129,10 +129,6 @@ class TestDiscretise(unittest.TestCase):
         self.assertIsInstance(un2_disc, pybamm.AbsoluteValue)
         self.assertIsInstance(un2_disc.children[0], pybamm.Scalar)
 
-        # parameter should fail
-        with self.assertRaises(NotImplementedError):
-            disc.process_symbol(pybamm.Parameter("par"))
-
     def test_process_complex_expression(self):
         var1 = pybamm.Variable("var1")
         var2 = pybamm.Variable("var2")
@@ -229,11 +225,7 @@ class TestDiscretise(unittest.TestCase):
 
     def test_core_NotImplementedErrors(self):
         # create discretisation
-        defaults = shared.TestDefaults1DMacro()
-        disc = shared.DiscretisationForTesting(
-            defaults.mesh_type, defaults.submesh_pts, defaults.submesh_types
-        )
-        disc.mesh_geometry(defaults.geometry)
+        disc = pybamm.BaseDiscretisation(None, None, None)
 
         with self.assertRaises(NotImplementedError):
             disc.gradient(None, None, {})
diff --git a/tests/test_expression_tree/test_concatenations.py b/tests/test_expression_tree/test_concatenations.py
index 04225588..f6f14871 100644
--- a/tests/test_expression_tree/test_concatenations.py
+++ b/tests/test_expression_tree/test_concatenations.py
@@ -128,6 +128,24 @@ class TestConcatenations(unittest.TestCase):
             ),
         )
 
+    def test_concatenation_orphans(self):
+        a = pybamm.Variable("a")
+        b = pybamm.Variable("b")
+        c = pybamm.Variable("c")
+        conc = pybamm.Concatenation(a, b, c)
+        a_new, b_new, c_new = conc.orphans
+
+        # We should be able to manipulate the children without TreeErrors
+        self.assertIsInstance(2 * a_new, pybamm.Multiplication)
+        self.assertIsInstance(3 + b_new, pybamm.Addition)
+        self.assertIsInstance(4 - c_new, pybamm.Subtraction)
+
+        # ids should stay the same
+        self.assertEqual(a.id, a_new.id)
+        self.assertEqual(b.id, b_new.id)
+        self.assertEqual(c.id, c_new.id)
+        self.assertEqual(conc.id, pybamm.Concatenation(a_new, b_new, c_new).id)
+
 
 if __name__ == "__main__":
     print("Add -v for more debug output")
diff --git a/tests/test_expression_tree/test_symbol.py b/tests/test_expression_tree/test_symbol.py
index 98b593f3..85b145ed 100644
--- a/tests/test_expression_tree/test_symbol.py
+++ b/tests/test_expression_tree/test_symbol.py
@@ -231,6 +231,17 @@ class TestSymbol(unittest.TestCase):
         self.assertFalse(algebraic_eqn.has_gradient())
         self.assertFalse(algebraic_eqn.has_divergence())
 
+    def test_orphans(self):
+        a = pybamm.Symbol("a")
+        b = pybamm.Symbol("b")
+        sum = a + b
+
+        a_orp, b_orp = sum.orphans
+        self.assertIsNone(a_orp.parent)
+        self.assertIsNone(b_orp.parent)
+        self.assertEqual(a.id, a_orp.id)
+        self.assertEqual(b.id, b_orp.id)
+
 
 if __name__ == "__main__":
     print("Add -v for more debug output")